Skip to content
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

Setup Rubocop #1537

Merged
merged 67 commits into from Jul 31, 2022
Merged

Setup Rubocop #1537

merged 67 commits into from Jul 31, 2022

Conversation

304
Copy link
Contributor

@304 304 commented May 5, 2019

As suggested in #1531

I would like to introduce rubocop to the project that should help to maintain a consistent code style.
The set of changes is quite big and it will take several PRs to finalize it.

The following PR:

  • adds rubocop gem
  • creates .rubocop.yml (for configurations)
  • makes changes as rubocop suggests (mostly by using --safe-auto-correct option)
  • disables multiple cops (it would be a gigantic PR if all of them are enabled at once)
  • adjusts Rakefile to perform rubocop checks by default

It enables Travis CI checks for the code style violations and that should already bring some value.
Next steps would be to re-enable cops from .rubocop.yml file until the check will pass without any warnings.

@namusyaka
Copy link
Member

To merge this, we need to carefully review the changes. I'd like sinatra helpers to review these changes.
/cc @jkowens @olleolleolle @mwpastore

@olleolleolle
Copy link
Member

@304 - Stellar work. Thank you so much for this! So many improvements to readability.

I have read the PR commit-by-commit, and its shows that you put in some thought into which change should come before the other. It was easy to follow along – each step was separate and followed from the earlier step. A story, if you will. Thanks for that, as well.

Solve the conflict, and then we can merge this, I think.


As for future work on RuboCop:

RuboCop has a concept of a TODO file - a list of remaining issues to be skipped. Having such a file would allow us to keep enabled the rules we want to follow for new code, while at the same time allowing the old code not to trouble that effort.

If some of the rules which didn't pass right now are desirable to pass in the future, we could use rubocop --auto-gen-config to create and include a file .rubocop_todo.yml, oh, I blogged about this workflow.

Copy link
Member

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please solve the Git conflict, so that we can go forward with this great improvement. Thank you!

@304
Copy link
Contributor Author

304 commented Jun 25, 2019

@olleolleolle Thank you for the review! 🎉
I've rebased the branch against master and it has no conflicts anymore.

I was not aware that Rubocop has a concept of a TODO file. It seems to be a really nice way how to proceed with future fixes. Thank you for sharing that!

@jkowens jkowens added this to the v2.1.0 milestone Mar 14, 2020
.rubocop.yml Outdated Show resolved Hide resolved
@olleolleolle
Copy link
Member

@304 👋 Hi!!!!

Would you mind rebasing again?

The note about required Ruby version 2.3+ is also relevant.

@namusyaka namusyaka modified the milestones: v2.1.0, v2.1.1 Sep 4, 2020
Gemfile Outdated Show resolved Hide resolved
@epergo
Copy link
Member

epergo commented Mar 3, 2021

Hello folks, could we push this? 🙏

If needed, I could continue with it

@jkowens jkowens removed the request for review from mwpastore March 3, 2021 14:11
@namusyaka
Copy link
Member

I'm thinking this can be merged into v2.1.1.

@jkowens jkowens force-pushed the feature_add_rubocop branch 2 times, most recently from ac5b509 to a07289e Compare July 28, 2022 15:29
@@ -121,26 +127,29 @@ def ping(timeout=30)
def alive?
3.times { get(ping_path) }
true
rescue Errno::ECONNREFUSED, Errno::ECONNRESET, EOFError, SystemCallError, OpenURI::HTTPError, Timeout::Error
rescue EOFError, SystemCallError, OpenURI::HTTPError, Timeout::Error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rubocop was reporting a ShadowedException. It seems SystemCallError would also rescue Errno::ECONNREFUSED, Errno::ECONNRESET errors.

lib/sinatra/indifferent_hash.rb Outdated Show resolved Hide resolved
sinatra-contrib/lib/sinatra/required_params.rb Outdated Show resolved Hide resolved
Copy link
Member

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, everyone, ❤️ ! This was a barn-raising, many hands needed.

@jkowens jkowens merged commit 8ae87a8 into sinatra:master Jul 31, 2022
@mvz
Copy link

mvz commented Sep 26, 2022

This seems to have resulted in an accidental breaking change for direct users of rack-protection: omniauth/omniauth#1089

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.

None yet

7 participants