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 EnforcePrefix option to Rails/Delegate #4452

Merged
merged 1 commit into from
Jun 6, 2017

Conversation

kstein413
Copy link
Contributor

@kstein413 kstein413 commented Jun 2, 2017

Hi! I'm proposing a change to the Rails/Delegate cop that adds an option (EnforcePrefix) to give users the ability to choose whether or not to enforce the prefix: true case when delegating.

At Betterment, we prefer to use

def foo_bar
  foo.bar
end

over

delegate :bar, to: :foo, prefix: true

because the second way makes it difficult to find where a method is defined (a search of the codebase would come up empty). I've defaulted the EnforcePrefix option to true, leaving current functionality as is, but this way the option is there to turn it off for projects that prefer to.


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).
  • Used the same coding conventions as the rest of the project.
  • 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.
  • All tests are passing.
  • The new code doesn't generate RuboCop offenses.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Updated cop documentation with rake generate_cops_documentation (required only when you've added a new cop or changed the configuration/documentation of an existing cop).

@kstein413 kstein413 force-pushed the kl/delegate-ignore-prefix-option branch 2 times, most recently from 0d357d5 to 058ff2d Compare June 2, 2017 02:58
@kstein413
Copy link
Contributor Author

@bbatsov I'm sure you're busy, but if you have a chance to take a look it’d be greatly appreciated!

@Drenmi
Copy link
Collaborator

Drenmi commented Jun 6, 2017

This is a great addition to the cop, and the code looks good. My only concern is that the option name might be a bit ambiguous. At first it sounds like it forces you to use prefix: true for all #delegate declarations. 🙂

Maybe EnforceForPrefixed or flip it and name it AllowPrefixed? What do you think @klesse413?

CHANGELOG.md Outdated
@@ -9,6 +9,7 @@
### Changes

* [#4444](https://github.com/bbatsov/rubocop/pull/4444): Make `Style/Encoding` cop enabled by default. ([@deivid-rodriguez][])
* [#4452](https://github.com/bbatsov/rubocop/pull/4452): Add option to Rails/Delegate for enforcing the prefixed method name case. ([@klesse413][])
Copy link
Collaborator

Choose a reason for hiding this comment

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

The name of the cop should be in backticks here.

# # good
# private
# def bar
# foo.bar
# end
#
# # EnforcePrefix: true
# # bad
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wouldn't nest this additionally, as it doesn't look well in the generated documentation.

# This cop looks for delegations that could have been created
# automatically with the `delegate` method.
#
# The EnforcePrefix option (defaulted to true) means that
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd put in backticks the name of the option and true.

# delegate :bar, to: :foo, prefix: true
#
# # EnforcePrefix: false
# # good
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

@bbatsov
Copy link
Collaborator

bbatsov commented Jun 6, 2017

because the second way makes it difficult to find where a method is defined (a search of the codebase would come up empty). I've defaulted the EnforcePrefix option to true, leaving current functionality as is, but this way the option is there to turn it off for projects that prefer to.

But what's the value of having this cop for you then? Prefixing unprefixed delegations?

@kstein413 kstein413 force-pushed the kl/delegate-ignore-prefix-option branch from 058ff2d to 2096d40 Compare June 6, 2017 15:58
… whether to enforce the prefix: true delegate case
@kstein413 kstein413 force-pushed the kl/delegate-ignore-prefix-option branch from 2096d40 to 9566e1b Compare June 6, 2017 16:03
@kstein413
Copy link
Contributor Author

@Drenmi I like the EnforceForPrefixed name, thanks!

@bbatsov The idea is that we would still like to use the cop to get a violation when using

def bar
  foo.bar
end

since we would prefer to use

delegate :bar, to: :foo

but we don't want to get a violation when using

def foo_bar
  foo.bar
end

so we'd like to be able to turn off enforcing that case

@bbatsov bbatsov merged commit faffd23 into rubocop:master Jun 6, 2017
@bbatsov
Copy link
Collaborator

bbatsov commented Jun 6, 2017

I thought as much. 👍

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