Fix double negation violations #1378
Conversation
| end | ||
| end | ||
|
|
||
| def self.normalize_path(path) |
There was a problem hiding this comment.
This method had been implemented as private before. Ruby doesn't actually allow for private class methods, so moving it above the private block and taking care of the violation doesn't change anything
|
Note a legit spec failure. |
lib/grape/path.rb
Outdated
|
|
||
| def uses_path_versioning? | ||
| !!(settings[:version] && settings[:version_options][:using] == :path) | ||
| (settings[:version] && settings[:version_options][:using] == :path) |
There was a problem hiding this comment.
@dblock
The error in the specs is related to this line change.
My problem is with the way ruby style-guide/rubocop addresses this violation.
# bad
!!(settings[:format] && Array(settings[:content_types]).size == 1)
#good
!(settings[:format] && Array(settings[:content_types]).size == 1).nil?
This raises the problem of if this operation returns false, it will evaluate to true, which makes it return the incorrect value.
I personally am not opposed to !! and think it is in fact better than the alternative !foo.nil?
Potentially, I could rework this PR to simply disable this check and move it to .rubocop.yml as disabled
Thoughts?
There was a problem hiding this comment.
I think keeping a double-negation is fine. I also don't agree with Rubocop here :)
ochagata
force-pushed
the
fix_double_negation_violations
branch
from
78d5fdc to
1ae8b7c
Compare
|
Alright, should be good once travis check finishes |
.rubocop.yml
Outdated
| - vendor/**/* | ||
| - bin/**/* | ||
|
|
||
| Style/DoubleNegation: |
There was a problem hiding this comment.
We often just run rubocop --auto-gen-config, can we just do that instead? It's more future-proof.
There was a problem hiding this comment.
Sure, but I'll have to keep this one in here so it ignores this
There was a problem hiding this comment.
Na, just remove it from here and let the auto-gen-config flag it as a violation. Maybe one day we'll change our mind on whether we like !! or not.
There was a problem hiding this comment.
Got it, I'll have this be a PR to fix the lint violation then
There was a problem hiding this comment.
Note that the other double-negation fixes are valuable, they just waste CPU cycles, so I am happy you did them!
ochagata
force-pushed
the
fix_double_negation_violations
branch
from
0ef64ee to
dd3ee60
Compare
|
|
||
| def uses_specific_format? | ||
| !!(settings[:format] && Array(settings[:content_types]).size == 1) | ||
| if settings.key?(:format) && settings.key?(:content_types) |
There was a problem hiding this comment.
Figured out why it was failing here and how to implement it properly.
Added a check for the presence of the keys, if either is nil, it will simply move on to the else block
|
Excellent. |
2 participants
3 style cops remain, otherwise all metrics cops