Reduce ignored rubocop offenses #653
Assignees
Comments
|
FYI: I have fixes for some of these offenses ready to submit as soon as #651 gets merged. These fixes include fixes for a spec test that is currently completely broken. I discovered this broken spec because it's code was causing some of the currently-ignored cop offenses I referenced above. |
rkoster
moved this from Inbox
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
1 task
|
This issue was marked as |
Justin-W
added a commit
to Justin-W/bosh-azure-cpi-release
that referenced
this issue
Jan 27, 2022
…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.
Justin-W
added a commit
to Justin-W/bosh-azure-cpi-release
that referenced
this issue
Jan 27, 2022
…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.
Justin-W
added a commit
to Justin-W/bosh-azure-cpi-release
that referenced
this issue
Jan 27, 2022
NOTE: 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. NOTE: 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. NOTE: Most of the fixed offenses were auto-fixed (via `rubocop -x`, `rubocop -a`, or `rubocop -A`, as appropriate). SQUASHED COMMIT HISTORY: --- rubocop: updated .rubocop_todo.yml (via `rubocop --regenerate-todo`) rubocop: Inline-disabled a `Style/ExplicitBlockArgument` offense (plus added code comments). NOTE: The offending method seems like a candidate for more/different refactoring beyond a fix for the `Style/ExplicitBlockArgument` offense. rubocop: Excluded 9 offenses: Style/NumericPredicate. 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. rubocop: Manually fixed: Style/IfUnlessModifier. NOTE: The 'Cop supports --auto-correct' auto-fixes resulted in harder-to-read code than before, warranting manual refactoring instead. rubocop: Force-auto-fixed: Style/StringChars. NOTE: This auto-fix required the use of the `rubocop --auto-correct-all` command. -A, --auto-correct-all Auto-correct offenses (safe and unsafe) 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. rubocop: Excluded 2 offenses: Style/HashTransformValues. 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. rubocop: Manually fixed 2 offenses: Lint/ToJSON. NOTE: The 'Cop supports --auto-correct' auto-fixes resulted in incorrect fixes for these offenses, requiring manual fixing instead. rubocop: Manually fixed 1 offense: Lint/RaiseException. 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. rubocop: Manually fixed 17 offenses: Lint/AmbiguousBlockAssociation. rubocop: Manually fixed 1 cop offense: Lint/AmbiguousBlockAssociation. Fix: set_disk_metadata_spec.rb: Fixed a broken spec. 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. 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. rubocop: Auto-fixed 16 offenses: azure_client.rb. rubocop: Excluded 6 offenses: Style/BisectedAttrAccessor. 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. rubocop: Excluded 2 offenses: Style/ArgumentsForwarding. 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. rubocop: Disable the `Style/TrailingUnderscoreVariable` cop. 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. rubocop: Inline-disabled the `Metrics/ClassLength` cop for all 12 classes 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. rubocop: Disable the `Metrics/BlockLength` cop for all '*_spec.rb' files. 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). Added 'TODO: Refactoring' comments: config.rb. 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. Minor layout refactoring: delete_spec.rb. NOTE: This refactoring makes the modified code section's layout more consistent with the project-wide layout conventions. Fix typo in comment: stemcell_manager2.rb
Not stale. The PR for this (#658) was previously blocked (on #651) until this morning. |
|
PR has been merged. |
Repository owner
moved this from Pending Merge | Prioritized
to Done
in Foundational Infrastructure Working Group
Feb 10, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Is your feature request related to a problem? Please describe.
Currently (as of #652), the
.rubocop_todo.ymlsettings are ignoring/excluding 6875 rubocop cop offenses. While many of these may be relatively harmless code layout or style conformance issues, some of the offenses may be indicators of existing bugs within the code base.Describe the solution you'd like
The ignored offenses should be resolved/fixed, and the
.rubocop_todo.ymlsettings updated appropriately, to reduce the number of ignored offenses as much as is feasible.Describe alternatives you've considered
You could continue to ignore the currently-ignored offenses.
Additional context
The rubocop scripts, dependencies, and settings were recently fixed (see #650, #651). Prior to those PRs, it seems that rubocop had not been used/maintained (for an undetermined period of time). Many of the currently-ignored offenses are likely the result of code changes made during and/or prior to this period when rubocop was unused/unusable (see #648).
Also, some of the current cop offenses are from optional and/or newer cops which were only recently enabled (see #650, #651) for this project. So those cops had never been applied to this code base until recently.
Rubocop stats
As of #652, there are:
The text was updated successfully, but these errors were encountered: