Skip to content

Upgrade Rubocop libraries#9201

Merged
ahukkanen merged 8 commits intodecidim:developfrom
i-need-another-coffee:ale-upgrade-rubocop
May 3, 2022
Merged

Upgrade Rubocop libraries#9201
ahukkanen merged 8 commits intodecidim:developfrom
i-need-another-coffee:ale-upgrade-rubocop

Conversation

@alecslupu
Copy link
Copy Markdown
Contributor

@alecslupu alecslupu commented Apr 28, 2022 •
edited by andreslucena
Loading

🎩 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
Copy Markdown
Contributor Author

alecslupu commented Apr 28, 2022

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

@alecslupu alecslupu mentioned this pull request Apr 28, 2022
Merged
@ahukkanen
Copy link
Copy Markdown
Contributor

ahukkanen commented Apr 29, 2022

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
Copy Markdown
Contributor Author

alecslupu commented May 2, 2022

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
Copy Markdown
Member

andreslucena commented May 2, 2022

Awesome, thanks for taking care of this one @alecslupu

@alecslupu alecslupu force-pushed the ale-upgrade-rubocop branch from 7f1c36d to fdca0a4 Compare May 2, 2022 08:10
@alecslupu
Copy link
Copy Markdown
Contributor Author

alecslupu commented May 2, 2022

@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.

This was referenced May 2, 2022
Merged
Merged
ahukkanen
ahukkanen previously approved these changes May 2, 2022
View reviewed changes
Copy link
Copy Markdown
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?

Comment thread decidim-generators/lib/decidim/generators/app_generator.rb Outdated
@alecslupu alecslupu requested a review from ahukkanen May 3, 2022 06:18
This was referenced May 3, 2022
Merged
Merged
@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
Merged
12 tasks
This was referenced May 4, 2022
Merged
Merged
Merged
@andreslucena andreslucena mentioned this pull request May 10, 2022
Closed
@alecslupu alecslupu added this to the 0.27.0 milestone Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahukkanen ahukkanen ahukkanen approved these changes

@andreslucena andreslucena Awaiting requested review from andreslucena

Assignees

No one assigned

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

Milestone

0.27.0

Development

Successfully merging this pull request may close these issues.

3 participants

@alecslupu @ahukkanen @andreslucena