Sat, 15 Jul 2017 18:29:06 +0200, /Robert Klemme/:
On 15.07.2017 17:14, Stanimir Stamenkov wrote:
I'm kinda disappointed in the capabilities of these tools.
You need to be aware of what is possible at all. Your expectations
seem unrealistic high to me.
If the given use case can't be covered by these tools my conclusion
of them not being really helpful past formatting style checks would
remain. I just want to find out if that's the case.
Most reports they produce are about suggested cosmetic
formatting, and few others which are all guidelines after all.
For example, RuboCop suggests class variables (vs. class instance
variables) should be avoided, which doesn't mean they can't have
legitimate usage.
But they should really be avoided. I have yet to see a problem that
is better solved with class variables than with instance variables
of the class (or even different means).
The specific use case was a base class which acts as a registry for
information provided by its sub types. The base class provides few
class methods for the registration into a class variable storage
(that one is protected), and then for reading the info publicly.
I guess I could make it use class instance variable at the expense I
should not forget to always invoke the "register" method with
explicit receiver (the base class) which I don't really want to
force, and it would be error prone for that reason.
I guess I could also make the `Base.register` method use `Base.instance_variable_set` but do you think that's anywhere better?
As far as I know "class instance variables" are quite specific to
Ruby, so avoiding "class variables" as seen in other languages at
all cost in favor to what I would call "side-effect class instance
variables" is confusing to me.
I've seen an experienced Ruby zealot blindly replacing such
occurrence causing quite substantial bug in the behavior.
Any code change can cause bugs (and especially when done with little involvement of the conscious mind). That is the simple truth.
True. This just a case where it shows the reports produced by these
tools could be deemed imprecise already, and should not be taken
lightly (better shut them up).
The examples I've given could be interpreted like:
some_hash[:a_key].strip.gsub!(/^0+/, '')
If not defined explicitly elsewhere `strip` is a String instance
method,
The difficulties start by "not explicitly defined elsewhere". The
method could be in a piece of code that is generated at runtime,
could be in C etc.
I'm aware dynamic definitions could lead to false positives, and I
think I'm suggesting a case where it is less likely to be an issue,
and more likely to gain advantage of flagging it, unless explicitly
shut up.
and it returns a _new_ string. I think a linter could produce
quite certain warning the `strip` result is not assigned, and
chained operations on it are mostly no-op (if that's not the last
statement of a method). Also the `gsub!` on a temporary string is
not likely having the desired effect.
You can say that because you probably have enough knowledge of the
context. But the fact that something is called "some_hash" does not guarantee at all that the variable points to a Hash instance. And
it goes on. Ruby is so dynamic that you can only know at runtime
what types are in play etc.
That's why I've suggested looking up class/method table of explicit definitions, and then the case I'm targeting is specifically about
the core String#strip (or String#gsub!) method. If there's no other
type in the project defining `strip` it is really likely the core
String#strip is used. The chain invocation of `strip.gsub!` could
further provide a hint to the linter of the actual types used. The
possibility to imply basic type info, even roughly, seems essential
to me for such tool to produce helpful reports.
Of course, you could produce warnings
like you suggest but then you'd have to wade through a huge pile of
false positives. And at the same time some real issues probably go undetected.
As far as the false positives are much fewer than the real problems
caught by such a report, shutting them up via the comment annotation
mechanism provided by the tool seems fine to me. The statement "I
would have to wade through a huge pile of false positives" is yet to
be determined.
Then, if similar statement/expression is used as a return value of
a method/block:
some_collection.map do |foo|
foo[:bar].strip.gsub!(/^0+/, '')
end
`gsub!` is a String instance method which has the peculiarity of
returning false,
No, nil.
Thanks for correcting.
instead of the original string when no substitution takes place.
This should also be flagged as a warning, as it is mostly likely a
mistake. It might not be a mistake if there's an alternative
provided:
... or if the programmer knows that there will always be at least
one substitution.
Whether a programmer is certain is questionable – that's the linter
purpose. It's better the programmer annotate this case explicitly
flagging his/her assurance of it. Then after the original
programmer is gone the annotation would remain as proof.
foo[:bar].strip.gsub!(/^0+/, '') || foo[:bar].strip
Of course this suggests the linter maintains some class/method
table, and has some knowledge of the core Ruby library. I'll see
if I could pursue for such an enhancement with the RuboCop
maintainers.
I wish you luck!
Thank you. It's not likely I'll do it too soon, not unless I could
prepare a formal proposal with more examples, and possible behavior.
--
Stanimir
--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)