Rubocop fixes for #653 #658
Conversation
NOTE: This refactoring makes the modified code section's layout more consistent with the project-wide layout conventions.
SEE: https://rubystyle.guide/#one-class-per-file NOTE: This file contains 9 "model" classes, of which only 1 is named "Config", and at least 4 of the 9 appear to be completely unrelated to the Config class. Also, several of the 9 classes have class-specific spec files (see: "src/bosh_azure_cpi/spec/unit/models/*_config_spec.rb"). So it seems like the code would be more maintainable if up to 8 of the 9 classes were split into separate files, with filenames corresponding to the class name and spec file name.
…sses over the default 100 lines. NOTE: It seems preferable to explicitly disable this cop (inline) for these classes, rather than setting a higher project-wide threshold (in .rubocop_todo.yml) since the threshold doesn't make reveal which classes are over the default threshold.
…les. NOTE: Since spec files tend to have long blocks. it seems preferable to explicitly disable this cop for those files, in order to be able to set a lower project-wide limit (which will apply to non-spec files). NOTE: Although 'shared_stuff.rb' is not technically a spec file, it contains rspec code, and should therefore be subject to similar rubocop config as the '*_spec.rb' files (see the NOTE above).
NOTE: Each of the existing offenses is less readable if the offense if fixed. So this seems like a cop which reduces readability, and should therefore be disabled project-wide.
NOTE: The offenses were permanently excluded (in '.rubocop.yml' instead of '.rubocop_todo.yml'), since fixing them would be backwards-incompatible with Ruby < v2.7, and it seems like the file containing the offenses _might_ need to be backwards-compatible with earlier versions.
NOTE: This cop was permanently disabled (in '.rubocop.yml' instead of '.rubocop_todo.yml') for 1 particular file, for reasons detailed in the comments, since using inline rubocop comments to exclude the 6 specific offenses did not prevent `rubocop -A` from auto-fixing those offenses. This could unintentionally prevent other (future, hypothetical) offenses in that same file from getting detected/fixed, but that seems preferable these 6 offenses getting unintentionally auto-fixed.
Offenses:
lib/cloud/azure/restapi/azure_client.rb:156:1: C: [Corrected] Layout/IndentationStyle: Tab detected in indentation.
while next_url != nil
^
lib/cloud/azure/restapi/azure_client.rb:156:2: C: [Corrected] Layout/IndentationConsistency: Inconsistent indentation detected.
while next_url != nil ...
^^^^^^^^^^^^^^^^^^^^^
lib/cloud/azure/restapi/azure_client.rb:156:8: C: [Corrected] Style/NonNilCheck: Prefer !next_url.nil? over next_url != nil.
while next_url != nil
^^^^^^^^^^^^^^^
lib/cloud/azure/restapi/azure_client.rb:156:10: C: [Corrected] Layout/IndentationConsistency: Inconsistent indentation detected.
while !next_url.nil? ...
^^^^^^^^^^^^^^^^^^^^
lib/cloud/azure/restapi/azure_client.rb:156:10: C: [Corrected] Style/NegatedWhile: Favor until over while for negative conditions.
while !next_url.nil? ...
^^^^^^^^^^^^^^^^^^^^
lib/cloud/azure/restapi/azure_client.rb:157:2: C: [Corrected] Layout/IndentationWidth: Use 2 (not 9) spaces for indentation.
@logger.debug("Getting resources from nextLink #{next_url}")
^^^^^^^^^
lib/cloud/azure/restapi/azure_client.rb:157:10: C: [Corrected] Layout/IndentationWidth: Use 2 (not 1) spaces for indentation.
@logger.debug("Getting resources from nextLink #{next_url}")
^
lib/cloud/azure/restapi/azure_client.rb:158:17: C: [Corrected] Layout/IndentationConsistency: Inconsistent indentation detected.
uri = URI(next_url)
^^^^^^^^^^^^^^^^^^^
lib/cloud/azure/restapi/azure_client.rb:159:17: C: [Corrected] Layout/IndentationConsistency: Inconsistent indentation detected.
response = http_get(uri)
^^^^^^^^^^^^^^^^^^^^^^^^
lib/cloud/azure/restapi/azure_client.rb:160:17: C: [Corrected] Layout/IndentationConsistency: Inconsistent indentation detected.
cloud_error("Got empty page from nextLink #{next_url}") if response.body.nil?
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/cloud/azure/restapi/azure_client.rb:162:17: C: [Corrected] Layout/IndentationConsistency: Inconsistent indentation detected.
body = JSON.parse(response.body)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/cloud/azure/restapi/azure_client.rb:163:17: C: [Corrected] Layout/IndentationConsistency: Inconsistent indentation detected.
result.deep_merge!(body)
^^^^^^^^^^^^^^^^^^^^^^^^
lib/cloud/azure/restapi/azure_client.rb:164:17: C: [Corrected] Layout/IndentationConsistency: Inconsistent indentation detected.
next_url = body['nextLink']
^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/cloud/azure/restapi/azure_client.rb:165:9: W: [Corrected] Layout/EndAlignment: end at 165, 8 is not aligned with while at 156, 1.
end
^^^
lib/cloud/azure/restapi/azure_client.rb:165:9: W: [Corrected] Layout/EndAlignment: end at 165, 8 is not aligned with while at 156, 9.
end
^^^
lib/cloud/azure/restapi/azure_client.rb:165:10: W: [Corrected] Layout/EndAlignment: end at 165, 9 is not aligned with until at 156, 8.
end
^^^
187 files inspected, 16 offenses detected, 16 offenses corrected
…y#653). NOTE: The fixed offenses were auto-fixed (via `rubocop -x`) incrementally, by re-enabling and fixing the offenses of one cop at each step (in the order indicated below). SQUASHED COMMIT HISTORY: --- rubocop: Auto-fixed: Layout/TrailingEmptyLines. rubocop: Auto-fixed: Layout/SpaceInsideHashLiteralBraces. rubocop: Auto-fixed: Layout/SpaceInsideArrayLiteralBrackets. rubocop: Auto-fixed: Layout/MultilineMethodCallIndentation. NOTE: The default `aligned` style is the closest match to the existing code. rubocop: Auto-fixed: Layout/LineEndStringConcatenationIndentation. rubocop: Auto-fixed: Layout/FirstHashElementIndentation. rubocop: Auto-fixed: Layout/FirstArgumentIndentation. rubocop: Auto-fixed: Layout/ClosingParenthesisIndentation. rubocop: Auto-fixed 2 cop offenses: Layout/ExtraSpacing, Layout/SpaceAroundOperators. rubocop: Auto-fixed 2 cop offenses: Layout/EmptyLinesAroundBlockBody. rubocop: Auto-fixed 1 cop offenses: Layout/EmptyLineBetweenDefs. rubocop: Auto-fixed 11 cop offenses: Layout/EmptyLinesAroundAttributeAccessor.
NOTE: This spec previously passed (despite an incorrect expectation) due to incorrect syntax. The incorrect syntax was one of the (previously-ignored) 'Lint/AmbiguousBlockAssociation' rubocop offenses.
NOTE: The offense was a false-positive. In this particular case, we must violate the cop's rule. So the offense is now disabled via an inline rubocop comment.
NOTE: The 'Cop supports --auto-correct' auto-fixes resulted in incorrect fixes for these offenses, requiring manual fixing instead.
NOTE: The offenses were permanently excluded (in '.rubocop.yml' instead of '.rubocop_todo.yml'), since fixing them would be backwards-incompatible with Ruby < v2.4, and it seems like the file containing the offenses _might_ need to be backwards-compatible with earlier versions.
…y#653). NOTE: The fixed offenses were auto-fixed (via `rubocop -a`) incrementally, by re-enabling and fixing the offenses of one cop at each step (in the order indicated below). SQUASHED COMMIT HISTORY: --- rubocop: Auto-fixed: Style/QuotedSymbols. rubocop: Auto-fixed: RSpec/Yield. rubocop: Auto-fixed: RSpec/ReceiveCounts. rubocop: Auto-fixed: RSpec/NotToNot. rubocop: Auto-fixed: RSpec/HookArgument. rubocop: Auto-fixed: RSpec/ExcessiveDocstringSpacing. rubocop: Auto-fixed: RSpec/EmptyLineAfterHook. rubocop: Auto-fixed: RSpec/EmptyLineAfterFinalLet. rubocop: Auto-fixed: RSpec/EmptyLineAfterExampleGroup. rubocop: Auto-fixed: RSpec/ContextMethod. rubocop: Auto-fixed: Lint/SymbolConversion. rubocop: Auto-fixed: Style/StringLiterals. rubocop: Auto-fixed: Style/SoleNestedConditional. NOTE: A few of the auto-corrected changes could potentially be simplified further by refactoring them to use the Hash#dig method instead of chained Hash#fetch calls (i.e. chained square brackets). rubocop: Auto-fixed: Style/SlicingWithRange. rubocop: Auto-fixed: Style/RedundantRegexpEscape. rubocop: Auto-fixed: Style/RedundantFileExtensionInRequire. rubocop: Auto-fixed: Style/RedundantBegin. rubocop: Auto-fixed: Style/RedundantAssignment. rubocop: Auto-fixed: Style/PercentLiteralDelimiters. rubocop: Auto-fixed: Style/NestedTernaryOperator. rubocop: Auto-fixed: Style/IfInsideElse. rubocop: Auto-fixed: Style/HashSyntax. rubocop: Auto-fixed: Style/HashEachMethods. rubocop: Auto-fixed: Style/HashConversion. rubocop: Auto-fixed: Style/FrozenStringLiteralComment. rubocop: Auto-fixed: Style/EmptyLiteral. rubocop: Auto-fixed: Style/CommentAnnotation. rubocop: Auto-fixed: Style/ClassEqualityComparison. rubocop: Auto-fixed: Style/CaseLikeIf. NOTE: The auto-fix also required that the Metrics/ModuleLength cop's Max (in .rubocop_todo.yml) be updated, to keep that cop from causing a new offense. rubocop: Auto-fixed: Style/BisectedAttrAccessor. NOTE: These are the remaining (non-excluded) offenses of this cop, and exclude the offenses which were manually excluded by an earlier commit. rubocop: Auto-fixed: Lint/AmbiguousRegexpLiteral. rubocop: Auto-fixed: Lint/AmbiguousOperatorPrecedence. rubocop: Auto-fixed: Lint/AmbiguousOperator.
NOTE: This auto-fix required the use of the `rubocop --auto-correct-all` command. -A, --auto-correct-all Auto-correct offenses (safe and unsafe)
NOTE: The 'Cop supports --auto-correct' auto-fixes resulted in harder-to-read code than before, warranting manual refactoring instead.
NOTE: The `Style/NumericPredicate` cop was reporting 9 auto-correctable offenses (for the modified lines). But auto-fixing the offenses caused failing specs, so those 9 cop offenses have been disabled inline.
…s added code comments). NOTE: The offending method seems like a candidate for more/different refactoring beyond a fix for the `Style/ExplicitBlockArgument` offense.
|
@klakin-pivotal Regarding the number of commits: I wasn't quite sure how much I should squash the commits in this PR. On the one hand, each rubocop cop's offenses represent a distinct set of issues to be resolved (and the fixes typically involve similar code changes, making them easy to review toether). On the other hand, all of the changes (whether auto-fixed or manual) have the same objectives (i.e. improve code correctness, maintainability, readability, etc.), but squashing together the fixes for multiple cops' offenses makes the code review more cognitively difficult. Therefore, I've prepared 3 different branches as options for you:
This PR is currently based on branch 'B', for the reasons noted above (and since it seemed like the probably-best option based on your prior feedback for a different PR). However, if you would prefer more or fewer commits, I'd be happy to revise the PR to the HEAD of either of the other 2 branches. |
|
So, I'm probably not the fellow you should be asking to review PRs that change Anyway. A few things:
So, yeah. Someone who's not me should actually review this PR, and if you dislike what I've said here, please do ignore it! (@Justin-W, can you request a review from someone, or do you need me to select someone? That is, do you see a thing in the upper-right-ish of the UI that says |
|
@klakin-pivotal Thanks for the response. Answers below.
Lol. Ok, fair enough. And I've had some similar frustrations, but have found that it does occasionally expose real issues (like the test which was broken but incapable of failing, and which I've fixed in this PR). And such "hidden bugs" were the primary reason for even attempting this PR (with code style consistency across the project being secondary).
I'm fine with that. But it sounds like you think maybe the "different reviewer" should give an opinion first? I really don't care either way, other than trying to minimize additional/wasted time spent on this PR.
I checked, and it looks like the ones that increased slightly were mostly increased because the
I do not have the permission to assign reviewers within this project, so you (or @rkoster) would need to make such assignments. (I have it on other orgs/projects, but not for this one.) Also, I only made this whole set of "fixes" with the intent to improve the state of the project. So if there are serious concerns about whether it actually does so, I'm open to hearing that. |
rkoster
requested review from
a team,
Benjamintf1 and
KaiHofstetter
and removed request for
a team
rkoster
requested review from
a team,
jpalermo and
beyhan
and removed request for
Benjamintf1,
KaiHofstetter and
a team
|
Thanks! @Justin-W |
This PR resolves many of the rubocop cop offenses (#653) which were previously-ignored due to the auto-generated config in .rubocop_todo.yml.
Almost all of the changes are "pure code refactoring", with zero impact to the behavior of the modified code (the notable exception being 1 unit test, which was found to be previously-broken, and is now fixed).
All of the fixed/excluded offenses were resolved incrementally (one cop at a time, in the order indicated below), by either auto-fixing the manually re-enabled offenses or else explicitly disabling/excluding (inline or in the .rubocop.yml, instead of in .rubocop_todo.yml) the previously-ignored offenses.
After each incremental change (whether auto-fixed by rubocop or manually fixed/resolved), the incremental code changes were re-reviewed (for both correctness and readability), and the unit test suite was re-executed (to verify no regressions were introduced). Where either the code review and/or unit tests indicated that the auto-fixed changes were unacceptable, the auto-fixed changes were discarded and manual fixes/resolution were applied instead.
Auto-fixes were performed via the
rubocop -x,rubocop -a, orrubocop -Acommands (in that order), depending on the auto-correct requirements for each individual cop.Checklist:
Please check each of the boxes below for which you have completed the corresponding task:
Please include below the summary portions of the output from the following 2 scripts:
NOTE: Please see how to setup dev environment and run unit tests in docs/development.md.
Unit Test output:
Rubocop output:
Changelog