Fix rubocop offenses #1350
Fix rubocop offenses #1350
Conversation
- Sort cops alphabetically in .rubocop.yml
- Fix Style/HashSyntax Offenses
- Satisfies Style/TrailingCommaInArguments, Style/TrailingCommaInArrayLiteral and Style/TrailingCommaInHashLiteral
- Layout/EmptyLines - Layout/EmptyLineAfterGuardClause - Layout/TrailingEmptyLines - Layout/EmptyLinesAroundBlockBody - Layout/EmptyLinesAroundModuleBody - Layout/EmptyLineAfterMagicComment - Layout/SpaceInsidePercentLiteralDelimiters - Layout/SpaceAroundEqualsInParameterDefault - Layout/SpaceInsideArrayLiteralBrackets - Layout/LeadingCommentSpace - Layout/SpaceBeforeComment - Layout/SpaceAroundOperators - Layout/SpaceInsideRangeLiteral - Layout/SpaceInsideReferenceBrackets - Layout/SpaceInLambdaLiteral - Layout/SpaceAfterComma - Layout/SpaceInsideHashLiteralBraces - Layout/TrailingWhitespace - Layout/ArgumentAlignment - Layout/HashAlignment - Layout/DotPosition - Layout/IndentationWidth - Layout/EndAlignment - Layout/MultilineOperationIndentation - Layout/IndentationConsistency - Layout/ClosingHeredocIndentation - Layout/MultilineMethodCallBrace- Layout - Layout/ClosingParenthesisIndentation
|
@mcmire do you prefer multiple PR's or a single one for all? |
Updated Layout/LineLength cop to Max 120 lenght and ignore everything inside specs/**/* Added rules for below cops in .rubocop.yml Lint/AmbiguousBlockAssociation Naming/HeredocDelimiterNaming Rails/SkipsModelValidations Style/FormatStringToken Fixed below mentioned cops: - Layout/CaseIndentation - Layout/DotPosition - Layout/ElseAlignment - Layout/IndentationWidth - Layout/LineLength - Layout/MultilineBlockLayout - Layout/MultilineOperationIndentation - Lint/AmbiguousBlockAssociation - Lint/MissingCopEnableDirective - Lint/NestedMethodDefinition - Lint/RedundantCopDisableDirective - Lint/RedundantRequireStatement - Lint/UnusedBlockArgument - Lint/UnusedMethodArgument - Metrics/ModuleLength - Naming/MemoizedInstanceVariableName - Naming/RescuedExceptionsVariableName - Rails/Output - Rails/Presence - Security/Eval - Security/Open - Style/ClassCheck - Style/CollectionMethods - Style/ConditionalAssignment - Style/EvalWithLocation - Style/FormatStringToken - Style/InverseMethods - Style/MutableConstant - Style/ParallelAssignment - Style/RedundantBegin - Style/RedundantCondition - Style/RedundantInterpolation - Style/RedundantSelf - Style/RedundantSort - Style/RescueStandardError - Style/SafeNavigation - Style/StringLiteralsInInterpolation - Style/SymbolProc
KapilSachdev
force-pushed
the
fix/rubocop_offenses
branch
from
4bc53c5
to
d87586c
Compare
|
Pending:
@mcmire How do you want to approach these? Should they be disabled? |
There was a problem hiding this comment.
Did a pass-through of the whole PR. In hindsight perhaps it would have been better as a succession of PRs, but we've already come this far, so it's not a big deal. I know I left a lot of comments, but let me see if I can summarize them:
- Can we stick to an 80-character line length? I know that seems small in the days of large screens, but that's what was being followed before (and on a laptop it's still not so small). :) Also, can we re-enable line length in spec files?
- When it comes to indentation, in general I (and to my knowledge this was a rule at thoughtbot, so it doesn't come from nowhere) try not to add more spaces than is necessary; instead the number of spaces should be an even number. What this means is that in cases where you have an assignment or are using an operator, and the right hand side needs to be spread across multiple lines, it's more maintainable, and an easier rule to remember IMO, to write:
foo <<
if bar
"baz"
else
"qux"
endrather than:
foo << if bar
"baz"
else
"qux"
end- In a similar vein, there are some changes around indentation of arguments to methods that span multiple lines. It has also been an unofficial rule that if any argument to a method is on its own line, then all arguments should be on their own lines. Is there any way to enforce this in Rubocop?
- There seem to be a lot of changes around trailing commas. This was a change I made to the Rubocop config a while back but which I now regret (it creates more of a pain point than it fixes). Considering there are a lot of files that now violate this rule, I'm wondering if that would be better to revert this cop (not sure how much trouble it is).
- There are a few cases where a long string was reformatted to fit across multiple lines but now contain line breaks that weren't there before, so make sure to change those back.
- There are a few false negatives that I ran across, so make sure to look out for those and change those back too.
lib/shoulda/matchers/action_controller/render_with_layout_matcher.rb
Outdated
Show resolved
Hide resolved
spec/unit/shoulda/matchers/active_record/association_matcher_spec.rb
Outdated
Show resolved
Hide resolved
lib/shoulda/matchers/active_record/validate_uniqueness_of_matcher.rb
Outdated
Show resolved
Hide resolved
spec/unit/shoulda/matchers/active_record/serialize_matcher_spec.rb
Outdated
Show resolved
Hide resolved
lib/shoulda/matchers/active_model/qualifiers/ignoring_interference_by_writer.rb
Outdated
Show resolved
Hide resolved
|
@KapilSachdev To answer your earlier questions:
|
|
@utkarsh2102 Thank you for the suggestions. I did notice a lot of changes around quotes and I agree that ideally double quotes make more sense and lead to more consistency. I'm not sure what the ratio of single quote to double quote usage is, though, so it might be a wash. Also, I would rather keep this PR focused to fixing existing violations as it relates to the current state of |
|
Thanks for reviewing. I will check those one by one. (Also I just noticed your comment that was made 4 days ago which i missed somehow) Below i'm responding against the comments made in general.
The code base has enforced style of
In master there are
Sure, I changed it to 120 thinking that
I will look what could be done in that regards as indentation sometimes conflicts with other cops
Sure, will do so.
All 4
|
|
Hello @KapilSachdev 👋🏻
Eeks, I am sorry for not looking more closely! In that case, this looks good, thanks for your work! 💪🏻 Hello @mcmire 👋🏻
Yep, why I say About the ratio, here's a good research study: rubocop/rubocop#5306 (comment) Let me know if you think it's worth migrating to
You're right! Let's tackle this later as a different PR (as proposed above^)! 👍🏻 |
|
@utkarsh2102 Hi just wanted to respond to your comment. I realize that most people probably use double quotes now but I'd rather not change the entire codebase to use double quotes. Although it may seem like there are bunch of changes to the code style in this PR, the code is really being aligned to fit rules that have already been in place (or enforce unofficial rules). It was bound to happen anyway so I treat this as a rip-the-bandaid-off kind of situation. But I'd rather not change the existing style at this point and force a widespread code change. I appreciate your suggestion though :) |
|
Hi @mcmire, No problem! |
|
@KapilSachdev Sorry for the delay in responding to your last comment. Here are my responses for the warnings you mentioned, first giving the link to the code, then the commentary: The warnings for these methods make sense — they are super long. I think refactoring them is out of scope, but it's probably worth noting. Maybe for now we want to disable BlockNesting and MethodLength for them and make a new issue to refactor them later? This one seems okay to me. There's no real way to make a constant inside of a block, because that constant will get hoisted outside that block, so I think our only choice is to have a method. I think disabling MethodLength is fine for this. I feel like there might be a way to refactor this slightly, but even so it will likely be over the limit still. Perhaps we disable MethodLength for this one as well. Maybe we ought to refactor this into different methods: This method is super long as well, so ideally it could be refactored later, but if removing the empty lines at the end keep it under the limit I'm fine with that. There might be a way to refactor this as well although it's not obvious, so removing the empty lines here makes sense. |
|
Thanks for the feedback @mcmire and no worries. I didn't get the time to work on it further and might not get this week also.
👍 I actually wanted to get hound or whatever we choose to get back up and running so that it will help cut your review cost a bit, and meanwhile we can refactor the remaining pieces.
I actually do prefer the code to be consistent with rules defined, around all areas of source code, be it either script/tasks, specs etc... but i think i wanted to get rid of those at that point. Its better we ignore them and add to |
|
@KapilSachdev Reading back over your comment, I realize this PR is pretty large so I'm fine with Excluding things for now if it helps to get this across the finish line. |
|
@mcmire Apologies for late reply. I will try to complete the suggestions you made soon and we can decide on next steps. |
|
@KapilSachdev No worries! Sounds good. |
KapilSachdev
force-pushed
the
fix/rubocop_offenses
branch
from
2694da2
to
2c3c6c9
Compare
|
Sorry. I saw the commit |
|
Just checked CI and it looks like there are a couple of failures. Perhaps one of the |
|
@vsppedro I should have added this in the commit message. This was not done in the commit you are mentioning, but in the changes pushed yesterday. @mcmire Yes, it is weird. The failing test should have failed in all the test suites, but they didn't and seems to be affected in postgresql adapter. class ValidateNumericalityOfMatcher
NUMERIC_NAME = 'number'.freeze
NON_NUMERIC_VALUE = 'abcd'.freeze # This seems to be the issue
DEFAULT_DIFF_TO_COMPARE = 1
...I will look into it. |
|
@KapilSachdev It looks like the test that's failing involves setting a value on an attribute which is a Postgres |
KapilSachdev
force-pushed
the
fix/rubocop_offenses
branch
from
2c3c6c9
to
f7adc8d
Compare
|
@mcmire As |
|
Upgrading to v1.0 in current PR state will add the following offences.
|
|
IMO, I think it would be best to let this upgrade for another PR. |
|
Yes, I'm working on it. Just mentioned the cops in case we want any cop to be disabled or need some non-default enforcement preference. |
|
@mcmire Please confirm the expectation for shoulda-matchers/lib/shoulda/matchers/doublespeak/object_double.rb Lines 21 to 26 in ffd5a28 For ERROR_MESSAGES = {
:> => { label: :greater_than,
assertions: [false, false, true], },
:>= => { label: :greater_than_or_equal_to,
assertions: [false, true, true], },
:< => { label: :less_than,
assertions: [true, false, false], },
:<= => { label: :less_than_or_equal_to,
assertions: [true, true, false], },
:== => { label: :equal_to,
assertions: [false, true, false], },
:!= => { label: :other_than,
assertions: [true, false, true], },
}.freezeand shoulda-matchers/lib/shoulda/matchers/active_record/association_matcher.rb Lines 1188 to 1199 in ffd5a28 MACROS = {
'belongs_to' => 'belong to',
'has_many' => 'have many',
'has_one' => 'have one',
'has_and_belongs_to_many' => 'have and belong to many',
}.freeze |
KapilSachdev
force-pushed
the
fix/rubocop_offenses
branch
2 times, most recently
from
5399f48
to
b18f576
Compare
There was a problem hiding this comment.
Adding some more comments. We're getting close. One thing I would say is that I feel like in some cases we are making code changes just to appease Rubocop but they actually detract from the readability or make things look objectively awkward. For instance, needing to add \\ to appease the line length. For now it's probably okay just so we can get this over the finish line, but maybe it's something we should look to relax in the future.
lib/shoulda/matchers/active_model.rb
Outdated
| @@ -4,12 +4,16 @@ | |||
| require 'shoulda/matchers/active_model/validation_matcher/build_description' | |||
| require 'shoulda/matchers/active_model/validator' | |||
| require 'shoulda/matchers/active_model/allow_value_matcher' | |||
| require 'shoulda/matchers/active_model/allow_value_matcher/attribute_changed_value_error' | |||
| require 'shoulda/matchers/active_model/allow_value_matcher/attribute_does_not_exist_error' | |||
| require 'shoulda/matchers/active_model/allow_value_matcher/'\ | |||
There was a problem hiding this comment.
I think this is one of those things that actually detracts from the readability of code, can we leave these alone or disable the cops on these?
There was a problem hiding this comment.
I will check if LineLength can be disabled for require. It should work i think.
| :<= => :less_than_or_equal_to, | ||
| :== => :equal_to, | ||
| :!= => :other_than, | ||
| :> => { label: :greater_than, |
There was a problem hiding this comment.
Can you format this as the following:
{
label: :greater_than,
assertions: [false, false, true]
},
...Again, abiding by the rule guideline that you shouldn't need to add extra spaces to align items.
| matcher.with_message(@message, values: { count: @value }) | ||
| matcher | ||
| end | ||
| @_submatchers ||= comparison_combos.map do | |
There was a problem hiding this comment.
This seems like another change to please Rubocop but actually detracts from the readability, would you mind changing this back?
There was a problem hiding this comment.
This change might have been caught up in the process.
| @@ -77,7 +83,7 @@ def all_bounds_correct? | |||
|
|
|||
| def failing_submatchers | |||
| submatchers_and_results. | |||
| reject { |x| x[:matched] }. | |||
| select { |x| !x[:matched] }. | |||
There was a problem hiding this comment.
I know I've asked about this before but which cop controls this? This seems like an unnecessary check.
There was a problem hiding this comment.
This was reverted to its original state.
Style/InverseMethods controls this behaviour.
There was a problem hiding this comment.
Yeah, sorry for the bluntness. I just think it may lead to churn.
| @@ -558,7 +558,8 @@ def add_submatcher(submatcher) | |||
| end | |||
|
|
|||
| if submatcher.respond_to?(:diff_to_compare) | |||
| @diff_to_compare = [@diff_to_compare, submatcher.diff_to_compare].max | |||
| @diff_to_compare = [@diff_to_compare, | |||
There was a problem hiding this comment.
Maybe:
@diff_to_compare = [
@diff_to_compare,
submatcher.diff_to_compare,
].maxAgain, the whole "keep indentation to multiple of 2 and don't add more than is necessary" sort of thing
| @@ -1,6 +1,7 @@ | |||
| require 'shoulda/matchers/active_record/association_matcher' | |||
| require 'shoulda/matchers/active_record/association_matchers' | |||
| require 'shoulda/matchers/active_record/association_matchers/counter_cache_matcher' | |||
| require 'shoulda/matchers/active_record/association_matchers/'\ | |||
There was a problem hiding this comment.
Another thing I don't think is necessary :)
| @@ -990,6 +990,13 @@ class AssociationMatcher | |||
|
|
|||
| attr_reader :name, :options | |||
|
|
|||
| MACROS = { | |||
There was a problem hiding this comment.
Would you mind putting this at the top of the class? I believe that we have been pretty good about putting them first.
| @@ -1292,7 +1299,8 @@ def class_name_correct? | |||
| end | |||
|
|
|||
| def join_table_correct? | |||
| if macro != :has_and_belongs_to_many || join_table_matcher.matches?(@subject) | |||
| if macro != :has_and_belongs_to_many || \ | |||
There was a problem hiding this comment.
When it comes to stretching if statements across multiple lines, what do you think about formatting this as:
if (
macro != :has_and_belongs_to_many ||
join_table_matcher.matches?(@subject)
)| @@ -33,9 +33,9 @@ def matches?(subject) | |||
| end | |||
|
|
|||
| missing_option << ( | |||
| 'fail validation if ' + | |||
There was a problem hiding this comment.
Is this enforced by a cop? I notice you doing this a lot and I'm wondering if it's your style or something that Rubocop is enforcing. If not no worries just curious.
There was a problem hiding this comment.
This change is enforced by 1.0, which requires string concatenation to be enforced with interpolation by default.
So to counter LineLength and Style/StringConcatenation cops, i had to use \.
I was thinking of heredoc, but that would be too much of a change, so updated it with \ for your feedback.
|
@KapilSachdev Let's leave both |
| @@ -67,7 +69,7 @@ def has_attribute? | |||
| end | |||
|
|
|||
| def has_expected_action_text? | |||
| @subject.send(rich_text_attribute).class.name == 'ActionText::RichText' | |||
| @subject.send(rich_text_attribute).instance_of?(ActionText::RichText) | |||
There was a problem hiding this comment.
Instead of using the class, can you try using a string?
@subject.send(rich_text_attribute).instance_of?('ActionText::RichText')Let's see if this change will fix the problem on CI. I almost sure. I'll make some tests.
EDIT 1:
instance_of does not work passing a string. I'll look other way.
EDIT 2:
defined?(ActionText::RichText) && @subject.send(rich_text_attribute).instance_of?(ActionText::RichText)But I think we can do better.
There was a problem hiding this comment.
class name evaluation with string poses an issue of being failed silently when the class is not even defined.
But this current scenario is not one of those cases so we may revert to string class comparison.
There was a problem hiding this comment.
I did not know that. Thank you!
- bump rubocop to v1.0 - Fix Layout/MultilineAssignmentLayout and other remaining offences - Exculde appraisal generated gemfiles in rubocop - Replace NON_NUMERIC_VALUE constant with instance method against failing test case in rails <= 5.1 and postgres adapter. This is a defect in rails where a frozen string is modified in https://github.com/rails/rails/blob/v5.1.7/activerecord/lib/active_record/connection_adapters/postgresql/oid/money.rb#L25
KapilSachdev
force-pushed
the
fix/rubocop_offenses
branch
from
b18f576
to
acfacaa
Compare
|
@KapilSachdev Anything else you noticed? I'm good with this PR if you are. |
|
@mcmire I think the current state of the PR is fine. |
|
@KapilSachdev Okay cool. I see you keep force-pushing, so did you want to keep your commits separate or put them together? |
|
@mcmire I had wanted to keep them separate, but if you think the change should go in one commit altogether, thats fine with me too. |
|
@KapilSachdev No, it's fine with me to keep them separate! Just wanted to know your preference. Merging now! |
|
Thank you. |
Ruby 3.4 will warn when `logger` is required without it being part of the gemspec: ruby/ruby@d7e558e The usage of a logger was added and removed in thoughtbot#1350
Ruby 3.4 will warn when `logger` is required without it being part of the gemspec: ruby/ruby@d7e558e The usage of a logger was added and removed in #1350
Helps move forward with #1345
Cop fixes are broken into multiple commits to ease reviewing.
Checklist of Fixed Cops: