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

Rename Cop to Base #962

Merged
merged 2 commits into from
Jul 16, 2020
Merged

Rename Cop to Base #962

merged 2 commits into from
Jul 16, 2020

Conversation

bquorning
Copy link
Collaborator

@bquorning bquorning commented Jul 10, 2020

This change is just so we follow the new naming pattern of RuboCop, which recently changed from RuboCop::Cop::Cop to RuboCop::Cop::Base. Here, we change RuboCop::Cop::RSpec::Cop to RuboCop::Cop::RSpec::Base.

This should not cause any user-observable changes, unless someone has their own custom cops inheriting from our base class. Should I add an entry to the changelog?

Would you prefer that I squash the two commits?


Before submitting the PR make sure the following are checked:

  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • [-] Added tests.
  • Updated documentation.
  • Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
  • The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

@bquorning bquorning changed the title Rename cop to base Rename Cop to Base Jul 10, 2020
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Awesome!

This should not cause any user-observable changes, unless someone has their own custom cops inheriting from our base class.

RuboCop kept Cop with a deprecation notice that Base should be used instead.
Maybe we should, too?

Should I add an entry to the changelog?

Yes, it's a notable change for those having custom cops.

@bquorning
Copy link
Collaborator Author

RuboCop kept Cop with a deprecation notice that Base should be used instead.
Maybe we should, too?

I think the RuboCop Cop is the old implementation with some deprecation notices. Our Cop implementation is already based on the new implementation (::RuboCop::Cop::Base). So the deprecation would only be regarding the name, not the implementation.

It’s quite easy for implementors to add an alias from Base to Cop, so I don’t think we should do it. It would just be pushing the problem further down the road.

I’ve added a notice in the changelog.

@pirj
Copy link
Member

pirj commented Jul 14, 2020

What I mean is that ::RuboCop::Cop::RSpec::Cop was renamed to ::RuboCop::Cop::RSpec::Base, and some custom cops, residing in third-party repos and in lib/rubocop/cops folders in certain projects will be broken with an update.
This cop, originated from an internal company's cop, would become broken with a minor version update.

I suggest keeping ::RuboCop::Cop::RSpec::Cop as an alias of ::RuboCop::Cop::RSpec::Base with a deprecation notice and removing it with 2.0.

@bquorning
Copy link
Collaborator Author

@pirj I’ve added ::RuboCop::Cop::RSpec::Cop back, and changed the Changelog notice.

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Thanks!

end
end
# @deprecated Use ::RuboCop::Cop::RSpec::Base instead
Cop = Base
Copy link
Member

Choose a reason for hiding this comment

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

should it produce some deprecation warning?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure how to do that. An initializer hook? E.g.

def initialize(*)
  warn '`::RuboCop::Cop::RSpec::Cop` is deprecate. Please use `::RuboCop::Cop::RSpec::Base` instead.'
  super
end

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if it's really necessary.

If you feel it is, the following will do (just in case not to rely on super):

class Cop < Base
  def self.inherited(base)
    warn '`::RuboCop::Cop::RSpec::Cop` is deprecated. Please use `::RuboCop::Cop::RSpec::Base` instead.'
  end
end

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking the other day what is the best way to track things that we'd like to drop/improve in a major version when RuboCop is 2.0, a separate ticket, a project, a 2.0 tag, a code annotation.

Anything would do I suppose. Do you have any preference?

I only have this deprecation and those two pulls in mind #940 (cop retired). Is there anything I'm missing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think a separate ticket and a 2.0 milestone (to tag issues/PRs) would do.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's time for rubocop-rubocop, containing cops on how to properly write cops, that could be used by extension developers :) Internal investigations brought to the next level ;)

Copy link
Member

Choose a reason for hiding this comment

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

This reminds me of a style-guide-style-guide :D

@bquorning bquorning mentioned this pull request Jul 15, 2020
4 tasks
@pirj
Copy link
Member

pirj commented Jul 15, 2020

@Darhazer please merge if you feel this is complete.

This change is just so we follow the new naming pattern of RuboCop, who
recently changd from `RuboCop::Cop::Cop` to `RuboCop::Cop::Base`. Here,
we change `RuboCop::Cop::RSpec::Cop` to `RuboCop::Cop::RSpec::Base`.

To make the commits easier to review, the superclass of all existing
cops will be changed in a separate commit.
@Darhazer Darhazer merged commit c45653e into master Jul 16, 2020
@Darhazer Darhazer deleted the rename-cop-to-base branch July 16, 2020 16:08
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.

3 participants