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

Fix for camelCase and unused argument name conflict #2759

Merged
merged 1 commit into from Feb 3, 2016

Conversation

mmcguinn
Copy link
Contributor

@mmcguinn mmcguinn commented Feb 2, 2016

Previously, the the behavior was something like the following:

foo      -> snake_case or camelCase
foo_bar  -> snake_case
fooBar   -> camelCase
_foo     -> unused snake_case
_foo_bar -> unused snake_case
_fooBar  -> invalid

New behavior should be:

foo      -> snake_case or camelCase
foo_bar  -> snake_case
fooBar   -> camelCase
_foo     -> unused snake_case or camelCase
_foo_bar -> unused snake_case
_fooBar  -> unused camelCase

Please let me know if there an appropriate place to add a test(s) for this and I'd be happy to add them. I looked at the existing tests and it doesn't look like they test any integration like this (and this change passes existing unit tests).

@mmcguinn
Copy link
Contributor Author

mmcguinn commented Feb 2, 2016

Forgot to explicitly link to the issue, #2758

@alexdowad
Copy link
Contributor

Please let me know if there an appropriate place to add a test(s) for this and I'd be happy to add them.

There should always be tests.

Which was the cop which was (erroneously) failing before? You could add a test case for that cop, which confirms that the code which previously failed doesn't fail any more.

@mmcguinn
Copy link
Contributor Author

mmcguinn commented Feb 2, 2016

I was originally thinking that a combination the Style/VariableName cop and the relevant Lint/UnusedMethodArgument, Lint/UselessAssignment or Lint/UnusedBlockArgument cop was responsible (which is why I was unsure where to put tests). Now that I come back to it though it doesn't really have anything to do with the lint cops directly. I'll add a couple tests to the VariableName cop.

@mmcguinn
Copy link
Contributor Author

mmcguinn commented Feb 2, 2016

Okay, I've added a pair of tests to the variable name cop and squashed all the commits down to one.

@bbatsov
Copy link
Collaborator

bbatsov commented Feb 3, 2016

Looks good. Just update the commit message to match our commit messages style - [Fix #n] Add optional .... See http://chris.beams.io/posts/git-commit/ for details.

@mmcguinn mmcguinn force-pushed the mm_2758 branch 2 times, most recently from fa245e0 to 13d5159 Compare February 3, 2016 16:07
@bbatsov
Copy link
Collaborator

bbatsov commented Feb 3, 2016

Would you mind rebasing this one more time? I didn't see your update and merged another PR before this one.

@mmcguinn
Copy link
Contributor Author

mmcguinn commented Feb 3, 2016

No problem - should be good to go once Travis confirms it again.

bbatsov added a commit that referenced this pull request Feb 3, 2016
Fix for camelCase and unused argument name conflict
@bbatsov bbatsov merged commit 0bfa753 into rubocop:master Feb 3, 2016
@bbatsov
Copy link
Collaborator

bbatsov commented Feb 3, 2016

👍

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

3 participants