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.

Sign up for GitHub

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

Jump to bottom

Fix registry deprecation notice #240

Merged
merged 1 commit into from
Jul 11, 2024
Merged

Fix registry deprecation notice #240

merged 1 commit into from
Jul 11, 2024

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Jul 10, 2024 •
edited
Loading

Closes #238

Morriar
Morriar reviewed Jul 10, 2024
View reviewed changes
Copy link
Contributor

@Morriar Morriar left a comment

Choose a reason for hiding this comment

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

I think the right approach would be to migrate to RuboCop::Cop::Base. See https://docs.rubocop.org/rubocop/v1_upgrade_notes.html.

@sambostock started the transition with #143.

@andyw8 andyw8 force-pushed the andyw8/fix-registry-deprecation-notice branch from 0acec34 to ab4676a Compare July 10, 2024 15:39
@andyw8
Copy link
Contributor Author

andyw8 commented Jul 10, 2024

Started work on moving to RuboCop::Cop::Base.

@koic
Copy link
Contributor

koic commented Jul 11, 2024

@andyw8 If you don't mind, I can open a PR to migrate to the V1 API for RuboCop::Cop::Base. In that case, it would be sufficient in this PR to just change Cop.registry to Registry.global. What do you think?

@andyw8
Copy link
Contributor Author

andyw8 commented Jul 11, 2024

@koic we started on that in 935bdf7 but if you'd like to take over, please go ahead.

koic added a commit to koic/rubocop-sorbet that referenced this pull request Jul 11, 2024
@koic
Make custom cop inherit RuboCop::Cop::Base
1e39ca2 
Follow up Shopify#240 (comment).

The legacy `Cop::Cop` API is deprecated and this PR use new `Cop::Base` API instead.

> maintain any RuboCop extensions, as the legacy API will be removed in RuboCop 2.0.

https://metaredux.com/posts/2020/10/21/rubocop-1-0.html
@koic koic mentioned this pull request Jul 11, 2024
Merged
@koic
Copy link
Contributor

koic commented Jul 11, 2024

@andyw8 Yeah, I've opened #241 to solve it. Since Cop.registry warning is being addressed in this PR, it is not handled in that PR.

@andyw8 andyw8 force-pushed the andyw8/fix-registry-deprecation-notice branch from 935bdf7 to ab4676a Compare July 11, 2024 18:46
@andyw8 andyw8 marked this pull request as ready for review July 11, 2024 18:47
@andyw8 andyw8 requested a review from a team as a code owner July 11, 2024 18:47
@andyw8 andyw8 requested review from Morriar and vinistock July 11, 2024 18:47
@andyw8
Copy link
Contributor Author

andyw8 commented Jul 11, 2024

Thanks. I've reset this branch to contain only the Cop.registry change.

@andyw8 andyw8 merged commit 4ab4032 into main Jul 11, 2024
20 checks passed
@andyw8 andyw8 deleted the andyw8/fix-registry-deprecation-notice branch July 11, 2024 19:55
koic added a commit to koic/rubocop-sorbet that referenced this pull request Jul 11, 2024
@koic
Make custom cop inherit RuboCop::Cop::Base
4222c34 
Follow up Shopify#240 (comment).

The legacy `Cop::Cop` API is deprecated and this PR use new `Cop::Base` API instead.

> maintain any RuboCop extensions, as the legacy API will be removed in RuboCop 2.0.

https://metaredux.com/posts/2020/10/21/rubocop-1-0.html
andyw8 pushed a commit that referenced this pull request Jul 11, 2024
@koic
Make custom cop inherit RuboCop::Cop::Base (#241)
4ee1f21 
Follow up #240 (comment).

The legacy `Cop::Cop` API is deprecated and this PR use new `Cop::Base` API instead.

> maintain any RuboCop extensions, as the legacy API will be removed in RuboCop 2.0.

https://metaredux.com/posts/2020/10/21/rubocop-1-0.html
paracycle
paracycle reviewed Jul 12, 2024
View reviewed changes
lib/rubocop/cop/sorbet/sigils/has_sigil.rb
@@ -15,7 +15,7 @@ module Sorbet
#
# If a `MinimumStrictness` level is specified, it will be used in offense messages and autocorrect.
class HasSigil < ValidSigil
@registry = Cop.registry # So we can properly subclass this cop
@registry = Registry.global # So we can properly subclass this cop
Copy link
Member

Choose a reason for hiding this comment

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

Is this even meaningful anymore? The comment seems to indicate that subclasses would not inherit the @registry instance variable and would thus break Cop.registry for subclass. Since we are moving to Registry.global, it feels like this should no longer be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#242

lib/rubocop/cop/sorbet/sigils/valid_sigil.rb
@@ -18,7 +18,7 @@ module Sorbet
# If an `ExactStrictness` level is specified, it will be used in offense messages and autocorrect.
# Otherwise, if a `MinimumStrictness` level is specified, it will be used in offense messages and autocorrect.
class ValidSigil < RuboCop::Cop::Cop # rubocop:todo InternalAffairs/InheritDeprecatedCopClass
@registry = Cop.registry # So we can properly subclass this cop
@registry = Registry.global # So we can properly subclass this cop
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

This was referenced Jul 12, 2024
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paracycle paracycle paracycle left review comments

@vinistock vinistock vinistock approved these changes

@Morriar Morriar Awaiting requested review from Morriar Morriar is a code owner automatically assigned from Shopify/sorbet

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New deprecation warning after upgrading rubocop to 1.65
5 participants
@andyw8 @koic @paracycle @Morriar @vinistock