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

Update rubocop dependency to < 2.0 #8

Merged
merged 11 commits into from Mar 29, 2021
Merged

Update rubocop dependency to < 2.0 #8

merged 11 commits into from Mar 29, 2021

Conversation

kitop
Copy link
Member

@kitop kitop commented Mar 27, 2021

First part of updating the required version of rubocop to unblock upgrades in applications that use this gem. This goes up to 0.89 before having to get into bigger changes.

I required some changes in custom assertions code when going to 0.87 so now tests will have to be run with latest version. It doesn't affect the cops, so it's safe to bump.

More updates to come in separate PRs.

Update: Merged #9. Now it supports up to < 2.0
One thing changed when updating to 0.90, we were using RESTRICT_ON_SEND in an unintended way and before 0.90 it was ignored. See rubocop/rubocop#8365

@kitop kitop added the type: chore work needed to keep the product and development running smoothly label Mar 27, 2021
@kitop kitop requested a review from a team as a code owner March 27, 2021 20:12
Remove RESTRICT_ON_SEND on InvalidModelAssignment cop since it wasn't
doing what we thought it did. Unitl < 0.90 it was ignored so we didn't
notice.
Update dependencies to rubocop < 2
@kitop kitop changed the title Update rubocop dependency to 0.89 Update rubocop dependency to < 2.0 Mar 27, 2021
needed = Hash.new { |h, k| h[k] = [] }
Array(cop.class.joining_forces).each { |force| needed[force] << cop }
forces = needed.map do |force_class, joining_cops|
force_class.new(joining_cops)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you comment more on why you needed to make this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yes, sorry! I thought it was in a commit comment but I missed it.

Change was introduced in rubocop/rubocop@f8813e7 on v0.87. Details are vague, because it's a big commit/PR, but it removes the join_force? method in favor of joining_forces)

Upgrade notes:
https://github.com/rubocop/rubocop/blob/3482522e2be022eee2321722f4e049ab4bcb3bf1/docs/modules/ROOT/pages/v1_upgrade_notes.adoc#joining-forces

In reality, I just copy-pasted the updated version from https://github.com/rubocop/rubocop-minitest/blob/v0.11.0/test/assertion_helper.rb (As noted in the top of this file, the entire file is copied from there)

next unless cop.join_force?(klass)

instances << klass.new([cop])
needed = Hash.new { |h, k| h[k] = [] }
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm... not sure why this is needed to be defined like this (v.s. needed = Hash.new) 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question! It's another Ruby nicety

This creates a hash where the default value is a new hash (new as in new every time, not the same array as it'd happen with Hash.new([]))
This allows to use << in the line below without having to initialize each needed[x] every time.

More details:

Copy link
Contributor

@keiko713 keiko713 left a comment

Choose a reason for hiding this comment

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

:magic:

@kitop kitop merged commit df16396 into master Mar 29, 2021
@kitop kitop deleted the update-dependencies branch March 29, 2021 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: chore work needed to keep the product and development running smoothly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants