-
Notifications
You must be signed in to change notification settings - Fork 4
Fix (or ignore) RuboCop offenses #2
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
Conversation
- Apart from `puts` and `yield` since those look weird with parentheses.
- RubyGems requires MFA for "popular" gems as of https://blog.rubygems.org/2022/08/15/requiring-mfa-on-popular-gems.html, but it's good practice even if not "popular".
rubocop.yml
Outdated
| - puts | ||
| - yield |
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 these should follow the rule. Do we have many of them to warrant an exception?
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.
In prawn-dev there are 7 yields and 2 puts. In prawn there are 70 yields and 11 puts.
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 the stats. Let's keep yield here but make puts follow the rule.
Ideally I'd like yield follow the rule, too, but it looks like a lot of work.
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.
Done! Sorry it took so long!
|
- I don't have enough expertise to document these, so I'll leave this as an exercise for the actual maintainers if they deem it necessary.
Hah, thanks for the review!
Oh yeah, that makes more sense. Done.
Great - just thought I’d check. |
|
All comments addressed. This, hopefully, should be good to go! (When the gem is bumped I’ll do a PR to |
|
@issyl0 Thank you. |
|
@issyl0 New gem version is up: https://rubygems.org/gems/prawn-dev/versions/0.5.0 |
bundle exec rubocopand fixed the ones it made sense to fix, disabling the ones it didn’t make sense to fix.RubyGems/RequireMFAchange which may change how you release this gem.Style/Documentation-related new offenses - adding docs is a TODO for someone more familiar with this codebase than me!