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

Prefix unused variables and arguments with _ #335

Merged
merged 1 commit into from
Jul 6, 2015
Merged

Conversation

gylaz
Copy link
Contributor

@gylaz gylaz commented Jun 16, 2015

No description provided.

@gfontenot
Copy link
Contributor

Fwiw, we typically use _foo to indicate an internal (private) variable, not an ignored variable in Swift/ObjC.

@georgebrock
Copy link
Contributor

What's the benefit of following this convention?

  • We aim to have short methods, if there's an argument that we need to accept but don't use, that should be clear from the method body without any special naming convention.
  • It doesn't make the code clearer to someone who isn't familiar with our style guide. As Gordon mentioned, an _ prefix is often used to mean "private" rather than "unused" (not just in iOS, but also in JavaScript and Python)
  • It can only be applied to positional arguments, not keyword arguments, which means we can't apply it consistently.

@gylaz
Copy link
Contributor Author

gylaz commented Jun 16, 2015

FWIW, this is in the Ruby specific section.

One situation I can think of:
Often, in specs folks create fixture data that isn't referenced in the test. The data is stored into a well-named clarity. Many of us use Syntastic in vim, and it will highlight an unused variable.

I'm not saying ^ is a good reason, but it does provide additional context.

@georgebrock
Copy link
Contributor

FWIW, this is in the Ruby specific section.

Sorry, I wasn't clear: what I meant was, I'm worried about taking a naming convention that's already going to be familiar to developers who know Swift/JavaScript/Python as meaning one thing, and using it to mean something different in Ruby.

@mcmire
Copy link

mcmire commented Jun 16, 2015

I have always thought that what Hound (or, rather, Rubocop) recommended to resolve an unused variable was a little unconventional. That said, I haven't had any issues with it.

A case in which I think it's useful is when overriding a method that takes an argument, but you never use that argument. The underscore, I think, is a good way of communicating that the variable is actually never used.

@jferris
Copy link
Contributor

jferris commented Jun 17, 2015

Rubocop and Syntastic recommend this because it's what Ruby recommends. Running Ruby with warnings will complain about unreferenced variables, but not if you prefix with an underscore, because that's a Ruby convention.

@JoelQ
Copy link
Contributor

JoelQ commented Jun 17, 2015

Haskell uses an underscore to denote unused arguments when pattern matching. It's pretty common to use a plain underscore to denote an unused block argument in Ruby.

@georgebrock
Copy link
Contributor

Running Ruby with warnings will complain about unreferenced variables, but not if you prefix with an underscore, because that's a Ruby convention.

I didn't know this, and before Hound started recommending it I'd never seen it in practice. Since it's the convention the interpreter wants us to follow, I'm fine with adding it to the guide.

@gabebw
Copy link
Contributor

gabebw commented Jun 17, 2015

I didn't know this, and before Hound started recommending it I'd never seen it in practice.

I'm fine with it, but, yes, I had never seen this before Hound recommended it.

@jyurek
Copy link

jyurek commented Jun 17, 2015

Underscore in Ruby has always meant that "this variable doesn't matter". I honestly don't care what a _ prefix means in other languages, because we're not using those languages here.

That said, the only time I've ever actually used it is in tests where I want something labelled but I also don't want to use it. This keeps syntastic (and Hound) from giving a warning.

I'm fine with not adding this as a guide since it's just as likely that the variable shouldn't have a name/exist if we don't intend to use it. But I'm fine if we do, too, since this is what Ruby wants.

@jferris
Copy link
Contributor

jferris commented Jun 17, 2015

It's pretty common to use a plain underscore to denote an unused block argument in Ruby.

Underscore methods are also treated specially in Ruby. You're allowed to use the same underscore name for multiple arguments:

> [1].each_with_index {|ignore,ignore| puts "hello" }
SyntaxError: (irb):2: duplicated argument name
[1].each_with_index {|ignore,ignore| puts "hello" }
                                   ^
    from /home/jferris/.rbenv/versions/2.2.2/bin/irb:11:in `<main>'
> [1].each_with_index {|_,_| puts "hello" }
hello
=> [1]
> [1].each_with_index {|_ignore,_ignore| puts "hello" }
hello
=> [1]

@mcmire
Copy link

mcmire commented Jun 17, 2015

Well, I learned something new today, too.

@@ -26,6 +26,7 @@ Ruby
calls.
* Prefer `if` over `unless`.
* Use `_` for unused block parameters.
* Prifix unused variables, or method arguments with `_`.
Copy link
Contributor

Choose a reason for hiding this comment

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

No comma.

Instead of "method arguments", how about "parameters"?

And "Prefix" is misspelled.

Prefix unused variables or parameters with underscore (_).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed.

@gylaz gylaz force-pushed the gl-prefix-unused branch from f25609f to 3406bfa Compare July 1, 2015 15:15
@thorncp
Copy link
Contributor

thorncp commented Jul 4, 2015

👍 🇺🇸

@gylaz
Copy link
Contributor Author

gylaz commented Jul 6, 2015

It sounds like we should merge this. Any objections @georgebrock?

@georgebrock
Copy link
Contributor

@gylaz No objections: after everything I've learnt in this PR discussion I'm actively in favour of merging it.

@gylaz gylaz force-pushed the gl-prefix-unused branch from 3406bfa to bef5980 Compare July 6, 2015 18:27
@gylaz gylaz merged commit bef5980 into master Jul 6, 2015
@gylaz gylaz deleted the gl-prefix-unused branch July 6, 2015 18:27
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.

10 participants