search

guard / listen

The Listen gem listens to file modifications and notifies you about the changes.
https://rubygems.org/gems/listen
MIT License
1.92k stars 245 forks source link

Rubocop Usage #504

Closed KapilSachdev closed 3 years ago

KapilSachdev commented 3 years ago

The repository uses rubocop for linting, but it seems that the offences are ignored rather being fixed. In the current state (c333f15) there are about 201 rubocop offences. Is there a reason for them to be ignored?

Also the version of rubocop can be updated to take advantage of newer cops.

ColinDKelley commented 3 years ago

I'm certainly guilty of dismissing Rubocop offenses most of the time...because most of them seem nit-picky and capricious, and some of them are just plain bad advice IMO. I'd like to see the cops tuned to a much smaller subset that we feel are actually important.

Here's a test I'd like to propose: we'll know Rubocop is tuned properly when its comments reflect what we would have typed in PRs ourselves, if we had had the time to be really thorough. Conversely: I'm not interested in hearing Rubocop's opinions if they don't match listen maintainer opinions.

@KapilSachdev Does it sound ok if we use this ticket for the purpose of tuning Rubocop so that the 201 offenses are a much smaller set that the listen maintainers actually feel are worth changing in this gem? If we get there, we could run rucop -a and let it make the changes, with the goal of getting to where there are 0 offenses on master.

KapilSachdev commented 3 years ago

Does it sound ok if we use this ticket for the purpose of tuning Rubocop

This ticket is created for the same purpose. Fixing offences after version upgrade would be a good approach as there might be some offences that might not be showing as offences anymore.

In current version, there are some cops that are used which are using wrong namespace or have unrecognized parameter, like Naming/FileName should be Style/FileName as the namespace was changed in 0.50 but we are on 0.49.

Current offences list: | Count | Cop | |-------|--------------------------------------------------| | 73 | Metrics/BlockLength | | 44 | Metrics/LineLength | | 21 | Style/SymbolArray | | 10 | Style/NumericLiteralPrefix | | 8 | Layout/MultilineMethodCallBraceLayout | | 8 | Style/AndOr | | 4 | Lint/AmbiguousBlockAssociation | | 4 | Style/NumericPredicate | | 4 | Style/RedundantFreeze | | 3 | Layout/AlignParameters | | 3 | Layout/DotPosition | | 3 | Layout/EmptyLinesAroundExceptionHandlingKeywords | | 2 | Layout/MultilineHashBraceLayout | | 2 | Style/RegexpLiteral | | 1 | Layout/MultilineMethodDefinitionBraceLayout | | 1 | Layout/SpaceInsideBlockBraces | | 1 | Lint/LiteralInCondition | | 1 | Lint/RescueException | | 1 | Style/FrozenStringLiteralComment | | 1 | Style/Lambda | | 1 | Style/MethodMissing | | 1 | Style/MultilineBlockChain | | 1 | Style/NegatedIf | | 1 | Style/SafeNavigation | | 1 | Style/TrailingCommaInLiteral | | 1 | Style/WordArray |

Upgrading will increase the count from 201 to 302 offences and would require to target ruby version in .rubocop.yml to 2.4 (2.3 is supported till 0.81). Some offences does not appear after upgrading because their default values have been changed, eg Layout/LineLength is changed from 80 to 120.

When upgraded to 1.0 | Count | Cop | |-------|--------------------------------------------------| | 73 | Metrics/BlockLength | | 21 | Style/SymbolArray | | 20 | Style/FormatStringToken | | 18 | Layout/EmptyLineAfterGuardClause | | 16 | Layout/HashAlignment | | 13 | Style/AccessorGrouping | | 10 | Style/NumericLiteralPrefix | | 8 | Layout/MultilineMethodCallBraceLayout | | 8 | Style/RescueStandardError | | 7 | Style/MutableConstant | | 6 | Lint/ConstantDefinitionInBlock | | 6 | Lint/RedundantRequireStatement | | 6 | Style/GlobalStdStream | | 6 | Style/RedundantRegexpEscape | | 5 | Naming/HeredocDelimiterNaming | | 5 | Style/HashAsLastArrayItem | | 5 | Style/MixinUsage | | 4 | Layout/LineLength | | 4 | Lint/AmbiguousBlockAssociation | | 4 | Style/NumericPredicate | | 4 | Style/RedundantFreeze | | 3 | Layout/ArgumentAlignment | | 3 | Layout/DotPosition | | 3 | Layout/EmptyLinesAroundExceptionHandlingKeywords | | 3 | Naming/MethodParameterName | | 2 | Layout/EmptyLinesAroundAttributeAccessor | | 2 | Layout/MultilineHashBraceLayout | | 2 | Lint/EmptyBlock | | 2 | Naming/RescuedExceptionsVariableName | | 2 | Security/Open | | 2 | Style/HashEachMethods | | 2 | Style/OptionalBooleanParameter | | 2 | Style/RegexpLiteral | | 2 | Style/SoleNestedConditional | | 1 | Gemspec/RequiredRubyVersion | | 1 | Layout/MultilineMethodDefinitionBraceLayout | | 1 | Layout/SpaceAroundMethodCallOperator | | 1 | Layout/SpaceInsideBlockBraces | | 1 | Lint/DuplicateMethods | | 1 | Lint/LiteralAsCondition | | 1 | Lint/MissingSuper | | 1 | Lint/MixedRegexpCaptureTypes | | 1 | Lint/NonDeterministicRequireOrder | | 1 | Lint/OrderedMagicComments | | 1 | Lint/RescueException | | 1 | Naming/MemoizedInstanceVariableName | | 1 | Style/CaseLikeIf | | 1 | Style/ConditionalAssignment | | 1 | Style/FrozenStringLiteralComment | | 1 | Style/Lambda | | 1 | Style/MissingRespondToMissing | | 1 | Style/MultilineBlockChain | | 1 | Style/NegatedIf | | 1 | Style/SafeNavigation | | 1 | Style/StderrPuts | | 1 | Style/StringConcatenation | | 1 | Style/WordArray |
ColinDKelley commented 3 years ago

I propose that we disable cops that have controversial opinions that are not widely agreed. There are many like that.

At our company we've been tuning our .rubocop.yml with this in mind, for many years. Here's where we have gotten to as of this week, on rubocop 0.91.1:

https://github.com/Invoca/style-guide/blob/master/ruby/.rubocop.yml

I'll make a PR with that merged in and see how many offenses we wind up with.

ColinDKelley commented 3 years ago

Merged and released in v3.3.2.