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

Upgrade Rubocop libraries #9201

Merged
merged 8 commits into from May 3, 2022

Conversation

alecslupu
Copy link
Contributor

@alecslupu alecslupu commented Apr 28, 2022

🎩 What? Why?

Upgrade

Gem                           Current     Latest      Requested  Groups
rubocop                       0.92.0      1.28.2
rubocop-rails                 2.9.1       2.14.2
rubocop-rspec                 1.43.2      2.10.0

♥️ Thank you!

@alecslupu alecslupu added dependencies Pull requests that update a dependency file or issues that talk about updating dependencies type: internal PRs that aren't necessary to add to the CHANGELOG for implementers labels Apr 28, 2022
@alecslupu
Copy link
Contributor Author

It appears that the upgrade itself has changed the standards or naming, revealing a lot of linting errors.

@ahukkanen , @andreslucena please advise on the following:

  1. Fix the violations as new Rubocop setup is instructing (becoming a very large PR)
  2. Adjust the settings of Rubocop to ignore the violations

@ahukkanen
Copy link
Contributor

It appears that the upgrade itself has changed the standards or naming, revealing a lot of linting errors.

@ahukkanen , @andreslucena please advise on the following:

  1. Fix the violations as new Rubocop setup is instructing (becoming a very large PR)
  2. Adjust the settings of Rubocop to ignore the violations

I would vote for option 1, i.e. fixing the violations. I ran rubocop against the whole codebase with these changes and there were 1397 violations of which 997 were automatically corrected. So that leaves 400 violations to correct which is quite a "chore" but doable.

We are not expecting you to do those changes unless you want to @alecslupu. We can also take this issue into the maintenance backlog.

The current RSpec rule block also needs to be fixed, as that's old syntax right now.

@alecslupu
Copy link
Contributor Author

It appears that the upgrade itself has changed the standards or naming, revealing a lot of linting errors.
@ahukkanen , @andreslucena please advise on the following:

  1. Fix the violations as new Rubocop setup is instructing (becoming a very large PR)
  2. Adjust the settings of Rubocop to ignore the violations

I would vote for option 1, i.e. fixing the violations. I ran rubocop against the whole codebase with these changes and there were 1397 violations of which 997 were automatically corrected. So that leaves 400 violations to correct which is quite a "chore" but doable.

We are not expecting you to do those changes unless you want to @alecslupu. We can also take this issue into the maintenance backlog.

The current RSpec rule block also needs to be fixed, as that's old syntax right now.

I can start working on this...

@andreslucena
Copy link
Member

Awesome, thanks for taking care of this one @alecslupu

@alecslupu
Copy link
Contributor Author

@ahukkanen I have updated the PR to add a list of disabled cops to help us through upgrade. I would not want to make this PR a 100+ files changed.
Once this PR is merged, i will move to fix cop by cop in a separate PRs.

ahukkanen
ahukkanen previously approved these changes May 2, 2022
Copy link
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

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

For me this approach seems perfectly fine.

Just one thing I did not completely understand so if you could explain @alecslupu I'd appreciate that.

@andreslucena Are you fine with the suggested approach fixing these issues?

decidim-generators/lib/decidim/generators/app_generator.rb Outdated Show resolved Hide resolved
@ahukkanen ahukkanen merged commit 1b116a4 into decidim:develop May 3, 2022
@alecslupu alecslupu deleted the ale-upgrade-rubocop branch May 3, 2022 08:23
@alecslupu alecslupu mentioned this pull request May 4, 2022
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file or issues that talk about updating dependencies type: internal PRs that aren't necessary to add to the CHANGELOG for implementers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants