-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: Update Rubocop and pull in recommended plugins #72
Conversation
Signed-off-by: Max VelDink <[email protected]>
Signed-off-by: Max VelDink <[email protected]>
@@ -20,9 +24,27 @@ Metrics/BlockLength: | |||
- 'spec/**/*.rb' | |||
- 'openfeature-sdk.gemspec' | |||
|
|||
Gemspec/DevelopmentDependencies: | |||
EnforcedStyle: gemspec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I typically don't use the development_dependency
style and favor using Gemfile
, but I opted to use what the project already uses.
RSpec/ContextWording: | ||
Enabled: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're using the OpenFeature specification language, disabling this rule as it will constantly be violated.
Enabled: false | ||
|
||
RSpec/MultipleExpectations: | ||
Max: 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always find the single default expecation max as overly punitive; I think 2 is a much more reasonable default.
Max: 2 | ||
|
||
RSpec/NestedGroups: | ||
Enabled: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, since we're using the specification language and nesting, I think having a maximum here is overly restrictive.
Enabled: false | ||
|
||
RSpec/SpecFilePathFormat: | ||
Enabled: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussion around this started in #71
Enabled: false | ||
|
||
RSpec/PendingWithoutReason: | ||
Enabled: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the pending output is readable enough without a reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this ✨ I have seen the suggestions, and keep forgetting to do something about them.
One thought I had while working on this is if the project would entertain moving to Standardrb instead of using stock Rubocop. I can add an issue for more discussion, but I am curious about any initial thoughts. Not maintaining an RSpec configuration is nice.
Totally would entertain it. I generally adopt that on new projects because we don't have to talk about rubocop config, except new additive things 😁
spec.add_development_dependency "rubocop", "~> 1.56" | ||
spec.add_development_dependency "rubocop-rake", "~> 0.6" | ||
spec.add_development_dependency "rubocop-rspec", "~> 2.24" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I think we can remove the versioning. Since we use a Gemfile.lock
in the repo, they won't update until they are explicitly bundle update
'd.
That makes it a touch easier to update w/o needing to update the gemspec.
@toddbaert hmm, it is the same error as #64 (comment) when trying to report, even with #67
|
@technicalpickles Great, I added codecov/codecov-action#74 and will take a stab at switching over to see how much work/reformatting would be involved. |
@technicalpickles the codecov thing ended up being a big can of worms: codecov/feedback#112. I'm reverting to an older version for now: #75 |
Closing this since codecov/codecov-action#76 was accepted |
This PR
While working on some specification work, I noticed that the version of Rubocop was far behind. Further, Rubocop recommended two extensions when running locally that we should consider including.
rubocop
and pulls inrubocop-rake
andrubocop-rspec
. I pessimistically locked to the minor version, as Rubocop is generally safe to have a looser versioning constraint in favor of benefiting from new rules sooner.Notes
I'll do my best to annotate this PR with the reason behind configuration or code changes; I'm open to discussing any fix here.
One thought I had while working on this is if the project would entertain moving to Standardrb instead of using stock Rubocop. I can add an issue for more discussion, but I am curious about any initial thoughts. Not maintaining an RSpec configuration is nice.
Follow-up Tasks
How to test
Clean Rubocop CI run