Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix rubocop issues #48

Merged

Conversation

jamiemccarthy
Copy link
Contributor

@jamiemccarthy jamiemccarthy commented May 11, 2022

I'm proposing this as an alternative to #39, basically just because #39 is several years old. (Changes to both this gem and rubocop cops make it easier to redo that work, in my opinion.)

This PR now passes bundle exec rake, both its rubocop check and its test suite. It's noisy but it passes. 🎉

I don't think any of these changes are unusual. I'm happy to discuss/defend my choices. I don't have strongly held opinions about any of this.

One change is fixing my own mistake with rescuing both an exception class and one of its subclasses (oops).

I would suggest switching CI from Travis to GitHub Actions as a more modern approach, and when doing that pick a range of ruby releases that fits with whatever required_ruby_version you pick. I picked >= 2.4 pretty arbitrarily. That version's been EOL for 2 years and supporting earlier versions is probably not necessary at this point.

For the first commit, I just ran rubocop --auto-correct and skimmed the resulting diff. Generally I trust rubocop's autocorrections when they are listed as safe. The remaining commits were all inspected carefully or hand-written.

This all ran with rubocop 1.29.0 except I think the first auto-correct ran with 1.26.0.

@jamiemccarthy jamiemccarthy force-pushed the jm-fix-rubocop-issues branch from d5505a1 to edb4003 Compare May 18, 2022 22:52
@jamiemccarthy
Copy link
Contributor Author

Happy to discuss any of these changes if that helps!

@shortdudey123
Copy link
Owner

Thank you very much! I will have to look into Github actions.

@shortdudey123 shortdudey123 merged commit 0f34496 into shortdudey123:master Jun 1, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants