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 cop Lint/UnexpectedBlockArity #9112

Merged
merged 1 commit into from Nov 29, 2020

Conversation

dvandersluis
Copy link
Member

Follows #9071 (comment):

Maybe it should be a different cop that checks the block arity for methods that are known to have arity 2. There are less of them now that Hash#each is being changed to arity 1. Maybe just Enumerable#each_with_object, min, max, minmax, slice_when, sort? Maybe we even have such a cop already?

This new cop checks for method blocks where the given block args don't match what the method is known to need. It is configurable using Methods, which is preloaded with all the Enumerable methods that take 2 args, as per the above comment.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • 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.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

@bbatsov
Copy link
Collaborator

bbatsov commented Nov 28, 2020

Overall the cop looks great. My only suggestion would be to ignore method calls without receivers, as those are going to be more likely to be false positives.

@dvandersluis
Copy link
Member Author

@bbatsov makes sense! I've updated it.

@bbatsov bbatsov merged commit 7927419 into rubocop:master Nov 29, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented Nov 29, 2020

Thanks!

@inkstak
Copy link

inkstak commented Dec 10, 2020

Would it be possible to add a deny list an allow list using regexp ?

We have a lot of sort blocks in jbuilder templates which are json properties.

json.sort do
  json.order     @current_order
  json.direction @current_order_direction
end

@dvandersluis
Copy link
Member Author

@inkstak configuring the cop like this probably would solve this for you:

Lint/UnexpectedBlockArity:
  Exclude:
    - '**/*.jbuilder'

@inkstak
Copy link

inkstak commented Dec 10, 2020

@dvandersluis Yes this is my current workaround, but it's too bad that this helpful cop doesn't cover regular Enum#sort methods present in our templates.

@dvandersluis dvandersluis deleted the lint/incorrect-arity branch December 10, 2020 16:27
@dvandersluis
Copy link
Member Author

There isn't really a way for rubocop to know if sort is Enumerable#sort or another sort. Generally when cops exclude methods it's done across the board, not based on receiver (because in general we can't tell what the receiver is).

@marcandre
Copy link
Contributor

(until we plug into rbs...)

@inkstak
Copy link

inkstak commented Dec 11, 2020

I actually suspect we can't tell what type is the receiver, but isn't it impossible to exclude cases using the name of the receiver or the whole line ? I humbly ask, the subtleties of rubocop parser are still beyonds me.

Lint/UnexpectedBlockArity:
  Allowlist:
    - 'json.sort'
    - /\Ajson\.sort do\Z/

Still, some real uses of Enumerable#sort could go through, but this should be more restricted

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