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

Add new Sorbet/RedundantExtendTSig cop #131

Merged
merged 1 commit into from
Feb 3, 2023
Merged

Add new Sorbet/RedundantExtendTSig cop #131

merged 1 commit into from
Feb 3, 2023

Conversation

sambostock
Copy link
Contributor

This cop is intended to be used in applications that have monkey patched T::Sig into all modules.

Module.include(T::Sig)

# later
module MyModule
  extend T::Sig
# ^^^^^^^^^^^^^ Do not redundantly `extend T::Sig` when it is already included in all modules.
  sig { void }
  def whatever = nil
end

It is disabled by default, as only application developers themselves can know if it applies to their app.

@sambostock sambostock requested a review from a team as a code owner February 2, 2023 23:01
Comment on lines 28 to 29
class RedundantExtendTSig < Base
extend AutoCorrector
Copy link
Contributor Author

@sambostock sambostock Feb 2, 2023

Choose a reason for hiding this comment

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

Looks like the way we generate docs doesn't support the Base API, and requires the old Cop API. I've updated it to the legacy approach to correction, and will open an issue to migrate.

Copy link
Contributor

@KaanOzkan KaanOzkan left a comment

Choose a reason for hiding this comment

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

I like the cop 👍 . You have a CI failure on Ruby 3+

@sambostock
Copy link
Contributor Author

Switched to positional arguments. I'll squash commits when I'm back at my computer.

What do you think of the offense message/docs?

Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

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

Small wording change to better emphasize which apps should use the cop.

lib/rubocop/cop/sorbet/redundant_extend_t_sig.rb Outdated Show resolved Hide resolved
config/default.yml Outdated Show resolved Hide resolved
@andyw8
Copy link
Contributor

andyw8 commented Feb 3, 2023

(for anyone else wondering: this is patched by shopify-types).

where it is useful to reduce noise.
Enabled: false
Safe: false
VersionAdded: '<<next>>'
Copy link
Member

Choose a reason for hiding this comment

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

I actually want to release the next version for a recent fix. Can we set this to 0.7.0 already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume Rubocop proper has tooling that does this automatically during the release process, but we haven't copied their tooling exactly (see #132). I think the expected workflow would be for the PR that bumps the version to do a find and replace of <<next>> with whatever version it's bumping the gem to.

@sambostock sambostock force-pushed the no-extend-t-sig branch 2 times, most recently from 0931663 to b646ef8 Compare February 3, 2023 16:25
@sambostock sambostock merged commit 49f9a04 into main Feb 3, 2023
@sambostock sambostock deleted the no-extend-t-sig branch February 3, 2023 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants