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 redundant self cop for arguments #484

Merged
merged 1 commit into from
Sep 18, 2013

Conversation

mbj
Copy link
Contributor

@mbj mbj commented Sep 13, 2013

IMHO using the self keyword is legit to evade a name clash with arguments.

Also I added a more verbose description of this cop and the nuances that must be taken into account when dealing with self.

Best,

Markus

name, _ = *node
@local_variables << name
end
alias_method :on_blockarg, :on_arg
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 suggest defining on_blockarg explicitly and calling the same helper method in both on_arg and on_blockarg. The alias_method pattern as used here is something we're moving away from.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh. Then I have some updating to do in code that I've contributed. 😄

@bbatsov
Copy link
Collaborator

bbatsov commented Sep 14, 2013

Nice work. Just update the Changelog, squash the commits and shorten the offending lines to get the build passing.

@mbj
Copy link
Contributor Author

mbj commented Sep 17, 2013

@bbatsov You insist of squashing the release? I'm totally happy with my series of commits tell the truth. And the truth is, I'm not able to do all changes at once.

@bbatsov
Copy link
Collaborator

bbatsov commented Sep 17, 2013

I like to have commits that represent a coherent unit of work - remove debug comments, fix docs, better naming, etc don't fit that bill. Such commits are just noise in the log IMHO and should be avoided whenever possible.

@bbatsov bbatsov mentioned this pull request Sep 18, 2013
@mbj
Copy link
Contributor Author

mbj commented Sep 18, 2013

@bbatsov I invalidated the true history, and our conversation here, via squashing the commits and force pushing the new one into this branch. Have to admit I dont like this, but your repo, your rules.

Hope this can be merged now!

bbatsov added a commit that referenced this pull request Sep 18, 2013
@bbatsov bbatsov merged commit 4d47bee into rubocop:master Sep 18, 2013
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