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

Allow by as method parameter in default config #5734

Conversation

AlexWayfer
Copy link
Contributor

Add by to AllowedNames in default configuration for cop
Naming/UncommunicativeMethodParamName in order to allow common
methods handling authorship with parameter update(values, by: user)

Replace this text with a summary of the changes in your PR.
The more detailed you are, the better.


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.

CHANGELOG.md Outdated
@@ -6,6 +6,10 @@

* [#5561](https://github.com/bbatsov/rubocop/issues/5561): Fix `Lint/ShadowedArgument` false positive with shorthand assignments. ([@akhramov][])

### Changes

* [#5734](https://github.com/bbatsov/rubocop/pull/5734): Change `Naming/UncommunicativeMethodParamName` add `by` to allowed names in default config. ([@AlexWayfer][])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you meant to write to add or something like this. Reads a bit odd right now.

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 just copied from #5626, sorry. Changed.

Add `by` to AllowedNames in default configuration for cop
Naming/UncommunicativeMethodParamName in order to allow common
methods handling authorship with parameter `update(values, by: user)`
@AlexWayfer AlexWayfer force-pushed the allow_short_method_param_by_in_default_config branch from 41e040f to ada54f1 Compare March 30, 2018 11:34
@Drenmi
Copy link
Collaborator

Drenmi commented Mar 30, 2018

Reiterating my comment:

Would it be meaningful to add some common preposition, such as "in", "on", "at", etc.?

😉

@AlexWayfer
Copy link
Contributor Author

AlexWayfer commented Mar 30, 2018

Reiterating my comment:

Reiterating comment by @unused:

I cannot think of an example using "in", "on", "at" except using some as pre-/suffixes. Happy to add if someone provides a legit code example :)

@Drenmi
Copy link
Collaborator

Drenmi commented Mar 30, 2018

Reiterating comment by @unused:

There's a real world example in this PR. 🙂

update(values, by: user)

Doesn't take much searching to come up with other examples from popular libraries, e.g.:

validate(method, on: action)
validates(:inclusion, in: collection)
mount(app, at: route)

@AlexWayfer
Copy link
Contributor Author

OK, thank you!

Added in the new commit.

Add `on`, `it` and `at` to AllowedNames in default configuration for cop
Naming/UncommunicativeMethodParamName in order to allow common
methods handling:

*   action with parameter `validate(method, on: action)`
*   collection with parameter `validates(:inclusion, in: collection)`
*   end-point with parameter `mount(app, at: route)`
@AlexWayfer AlexWayfer force-pushed the allow_short_method_param_by_in_default_config branch from 53a3d87 to 325cb7b Compare March 30, 2018 14:00
@@ -769,6 +769,10 @@ Naming/UncommunicativeMethodParamName:
- io
- id
- to
- by
- 'on'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this in quotes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because without them it's Boolean (true).

Example.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is part of Psych. on, off, yes, no, y and n are all converted too boolean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Drenmi Firstly, it's part of YAML standard.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Somehow I never noticed this. Now I'm enlightened. 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bbatsov I forgot about this, but tests failed without quoting. 😁

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's consider quoting all of them. Consistency is nice, and it'll prevent this question from recurring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mikegee Yes, maybe you're right. I thought about this, but didn't dare.

@bbatsov bbatsov merged commit 908fc3b into rubocop:master Mar 31, 2018
@AlexWayfer AlexWayfer deleted the allow_short_method_param_by_in_default_config branch April 2, 2018 09:09
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.

4 participants