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

Use mixin instead of superclass for signature cops #183

Merged
merged 1 commit into from
Oct 16, 2023
Merged

Use mixin instead of superclass for signature cops #183

merged 1 commit into from
Oct 16, 2023

Conversation

sambostock
Copy link
Contributor

@sambostock sambostock commented Oct 12, 2023 •
edited

This replaces the SignatureCop parent class with a SignatureHelp mixin module. Since SignatureCop was inheriting from the deprecated RuboCop::Cop::Cop, the cops that were inheriting from SignatureCop are updated to inherit directly from Cop instead.

This approach

  • matches what RuboCop does internally,
  • avoids the need to override the registry (?), and
  • allows granularly migrating from the deprecated Cop class to Base.

Before Merging

@sambostock sambostock requested a review from a team as a code owner October 12, 2023 18:58
@sambostock sambostock mentioned this pull request Oct 12, 2023
Merged
sambostock
sambostock commented Oct 12, 2023
View reviewed changes
lib/rubocop/cop/sorbet/signatures/enforce_signatures.rb Outdated
@@ -93,11 +93,11 @@ def check_node(node)
end

def param_type_placeholder
cop_config["ParameterTypePlaceholder"] || "T.untyped"
cop_config["ParameterTypePlaceholder"] || "T.untyped" # rubocop:todo InternalAffairs/UndefinedConfig
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to a bug, it doesn't seem possible to make this cop pass, so I'm disabling it inline.

#184 will disable it entirely until the bug is fixed, at which point we can address all the offenses.

sambostock
sambostock commented Oct 12, 2023
View reviewed changes
lib/rubocop/cop/sorbet/signatures/checked_true_in_signature.rb
Comment on lines -3 to -4
require "rubocop"
require_relative "signature_cop"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cops shouldn't need to require rubocop or any mixins, as those should already be required by lib/rubocop-sorbet.rb and lib/rubocop/cop/sorbet_cops.rb, respectively.

@sambostock sambostock force-pushed the signature-help branch 2 times, most recently from 715b4b2 to e3a2905 Compare October 12, 2023 20:32
sambostock
sambostock commented Oct 12, 2023
View reviewed changes
lib/rubocop/cop/sorbet/signatures/signature_build_order.rb Outdated
Comment on lines 12 to 31
#
#
class SignatureBuildOrder < ::RuboCop::Cop::Cop # rubocop:todo InternalAffairs/InheritDeprecatedCopClass
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This cop is currently undocumented. Without the empty comments, YARD uses the rubocop:disable comment as documentation, which makes it into manual/cops_sorbet.md. Adding these explicitly provides empty documentation, which maintains the status quo, as documenting it is out of scope of this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out this approach didn't work, and neither did the other things I tried, so I just documented it in #186.

This was referenced Oct 13, 2023
Merged
Merged
@sambostock sambostock changed the base branch from main to document-sig-build-order October 13, 2023 15:33
Morriar
Morriar reviewed Oct 13, 2023
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.

Let's rebase this on top of #184 to avoid all the disable comments?

@sambostock
Copy link
Contributor Author

Right, I did intend to do that. 👍

This PR branches off #186. If we can get that in first, I can do it in a single rebase.

Base automatically changed from document-sig-build-order to main October 15, 2023 22:20
@sambostock
Use mixin instead of superclass for signature cops
f1502f0 
This approach
- matches what RuboCop does internally,
- avoids the need to override the registry (?), and
- allows granularly migrating from the deprecated `Cop` class to `Base`.
@sambostock
Copy link
Contributor Author

@Morriar I've rebased and removed the rubocop:disable comments. 👍

@sambostock sambostock merged commit 73882bb into main Oct 16, 2023
12 checks passed
@sambostock sambostock deleted the signature-help branch October 16, 2023 01:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Morriar Morriar Morriar left review comments

@paracycle paracycle paracycle approved these changes

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

@vinistock vinistock Awaiting requested review from vinistock vinistock 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.

None yet
3 participants
@sambostock @paracycle @Morriar