I've tried RuboCop and RubyCritic but they don't seem to flag the given usages as problems. Is there another tool to help with this, or is
there specific configuration to any of the two mentioned which would
trigger the intended warnings?
On 14.05.2017 14:38, Stanimir Stamenkov wrote:
I've tried RuboCop and RubyCritic but they don't seem to flag the given
usages as problems. Is there another tool to help with this, or is
there specific configuration to any of the two mentioned which would
trigger the intended warnings?
This is hard to do correct as it requires knowledge about how a
method works. At best, this would only be a rough guideline. It
starts by reliably detecting the class of the receiver.
Mon, 15 May 2017 08:19:03 +0200, /Robert Klemme/:
On 14.05.2017 14:38, Stanimir Stamenkov wrote:
I've tried RuboCop and RubyCritic but they don't seem to flag the given
usages as problems. Is there another tool to help with this, or is
there specific configuration to any of the two mentioned which would
trigger the intended warnings?
This is hard to do correct as it requires knowledge about how a method
works. At best, this would only be a rough guideline. It starts by
reliably detecting the class of the receiver.
I'm kinda disappointed in the capabilities of these tools.
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.
I've seen an experienced
Ruby zealot blindly replacing such occurrence causing quite substantial
bug in the behavior.
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,
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.
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,
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:
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.
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.
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).
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.
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.
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.
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.
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.
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.
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!
Sysop: | Keyop |
---|---|
Location: | Huddersfield, West Yorkshire, UK |
Users: | 285 |
Nodes: | 16 (2 / 14) |
Uptime: | 76:19:48 |
Calls: | 6,489 |
Files: | 12,096 |
Messages: | 5,276,215 |