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

Remove Style/ExtendSelf cop #5233

Merged
merged 2 commits into from
Dec 13, 2017
Merged

Conversation

pocke
Copy link
Collaborator

@pocke pocke commented Dec 13, 2017

Style/ExtendSelf was introduced by #5108, but the cop duplicates with Style/ModuleFunction.
So this change reverts the commit (da69b88).


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Run rake default or rake parallel. It executes all tests and RuboCop for itself, and generates the documentation.

`Style/ExtendSelf` was introduced by rubocop#5108, but the cop duplicates with `Style/ModuleFunction`.
So this change reverts the commit (da69b88).
@bbatsov
Copy link
Collaborator

bbatsov commented Dec 13, 2017

That's new. 🤣 Did you compare which one was implemented better?

@pocke
Copy link
Collaborator Author

pocke commented Dec 13, 2017

I didn't compare them. I thought the new cop should be removed for compatibility, so I removed it. Maybe we should work on another pull-request to improve the cop's implementation.

@Drenmi
Copy link
Collaborator

Drenmi commented Dec 13, 2017

Whoops! 😬

@bbatsov bbatsov merged commit c3ceec9 into rubocop:master Dec 13, 2017
@bbatsov
Copy link
Collaborator

bbatsov commented Dec 13, 2017

I didn't compare them. I thought the new cop should be removed for compatibility, so I removed it. Maybe we should work on another pull-request to improve the cop's implementation.

@Drenmi Can you look into this? I wonder how you didn't notice the existing cop. 😆 I'd assumed you'd be getting some offenses from it as well. Perhaps you're just doing TDD. 😉

@flyerhzm
Copy link
Contributor

Style/ExtendSelf has an autocorrect method while Style/ModuleFunction does not, looks we should remove Style/ModuleFunction cop instead.

@pocke pocke deleted the good-bye-Style/ExtendSelf branch December 16, 2017 01:49
@pocke
Copy link
Collaborator Author

pocke commented Dec 16, 2017

Style/ExtendSelf has an autocorrect method while Style/ModuleFunction does not, looks we should remove Style/ModuleFunction cop instead.

Maybe we can move the auto-correction to Style/ModuleFunction. 👍
And I think we can't remove Style/ModuleFunction, because some users configure the gem already. So if remove the cop, it's a breaking change.

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.

None yet

4 participants