Skip to content

Conversation

@mjankowski
Copy link
Contributor

This is an attempt at resolving #72

As noted in that issue, I only vaguely understand the rubocop rule loading and validating process -- and then both rubocop-rails and standard-rails do some amount of overriding/modifying that process. Anyway, this is a first pass and maybe it's fine or maybe we can find something more elegant.

Concerns here:

  • This change is inspired by - https://github.com/rubocop/rubocop-rails/blob/v2.33.3/lib/rubocop/rails/plugin.rb#L27-L32 - which is doing the same sort of warning silencing from within rubocop rails. I don't love the idea of borrowing something which calls itself a dirty hack.
  • I'm not actually sure how to produce these warnings from the standard rails test suite itself (I only see it from the full loading process initiated by a separate app), so I'm not sure what test to even add here to avoid regression and/or let us make this more elegant in the future
  • I don't really understand how we DONT see a warning for TargetRailsVersion (the one worked around in rubocop-rails), since (I think?!) we are overriding the load process such that the rubocop-rails #rules method never gets called, and so, one would think, whatever issue that one is working around would need to be re-worked-around?

Benefits:

  • If rubocop-rails is doing this internally, maybe it's good enough? (and presumably if/when some cleaner solution emerges we could borrow it?)
  • This does indeed silence that output from a local app which otherwise sees the issue, when I update to point at local repo of standard rails with this change

Random side adventure during this journey ... in the other part of this diff, where I added Lint/UselessAccessModifier to the existing list to disable for similar reasons, there's some small inconsistency with the comment there and the rules and the history of that whole thing, I think?

  • The comment says standard does not enable them to begin with, which is true for Style/InvertibleUnlessCondition, but not true for Lint/SafeNavigationChain which as of https://github.com/standardrb/standard-rails/pull/22/files#diff-da3213a24b350fa386ff3bab0d6b239112408c3fe4f986cbd9d5236f7e0b61e4R31-R40 is enabled
  • The commit which added this "without extended" - acccb02 - removes Lint/RedundantSafeNavigation and Style/InvertibleUnlessCondition from the base config, but then adds Style/InvertibleUnlessCondition and Lint/SafeNavigationChain to this reject line ... should that latter addition have actually been Lint/RedundantSafeNavigation (to match what was removed), and/or should they all be there?
  • At minimum maybe that comment needs an update.

Thoughts on all topics welcome.

@mjankowski
Copy link
Contributor Author

Ah -- funny timing, basically dupe of #97

Will leave open until that's reviewed.

@mjankowski
Copy link
Contributor Author

Added one of the tests from linked PR, which helped expose the "why wouldnt we have to ignore target version as well?" issue.

Updated to include that test, and a change needed to also remove target version thing.

@jasonkarns
Copy link
Contributor

Thank you for this! Just merged #97, though, so closing this one.

@jasonkarns jasonkarns closed this Sep 24, 2025
@mjankowski
Copy link
Contributor Author

Thanks.

Any thoughts on my last note? Should I open separate issue for that.

@jasonkarns
Copy link
Contributor

jasonkarns commented Sep 24, 2025

@mjankowski yes, a separate issue would be welcome!! apologies if I jumped too quickly to close

@mjankowski
Copy link
Contributor Author

No problem, I would have closed this when the other merged anyway.

Will attempt to turn the relevant bits into a coherent issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants