Don't write code that generates warnings

[croaky]

No description provided.

[mcmire]

I agree with this, although it basically implies that you're either running tests with the --warnings flag on, and/or that you're using the syntastic plugin. Both of these seem like reasonable assumptions, but I just wanted to point that out.

[croaky]

Yup, good point. At least a couple of Rubocop's warnings (which we also catch with Hound) are designed based on ruby -cw: ShadowingOuterLocalVariable, UselessAssignment.

[pbrisbin]

👍 from me. I agree that actually ensuring you're notified of warnings as you work isn't as easy in Ruby as other languages, but I think that shouldn't stop us from having this explicit guide anyway.

[derekprior]

I like this too. Running your rails app with -w is going to be painful because Ruby libraries are historically bad at this but it's a good goal. The guideline is valuable without it though... "Don't do this because I know it to produce a warning" is fine feedback.

[jferris]

Is there a good list of things which will generate warnings in Ruby? Since you can't actually run with the warnings on, I think we either need to reference the list or list them out ourselves.

[sgrif]

I don't think there's a full list anywhere that I know of. Easiest way is to add -w to your test task for whatever gem you're working on. These three account for the vast majority of warnings that I run into.

• Uninitialized instance variable
  • Solution: Add a defined? check or explicitly set it to nil in the constructor. ||= is fine with undefined variables
• Private attribute?
  • Solution: Make it protected
• * interpreted as argument prefix
  • Solution: Use parenthesis by default

Ruby 2.2 also introduced an annoying new one "possible reference to past scope" which appears to occur any time you assign a variable with the same name as an argument for any block which existed in the same scope. Haven't seen it enough to know the full details of it, but there's an issue on redmine about it being hurtful in certain contexts.

[croaky]

Here's an article that talks about how Ruby's verbose mode is broken:

http://mislav.uniqpath.com/2011/06/ruby-verbose-mode/

[mcmire]

Yeah, I know from shoulda-matchers that warnings are painful and no one likes to turn them on because they're everywhere. I have them enabled but I actually had to write something to basically hide all non-related-shoulda warnings.

Anyways, 👍 from me for this guideline.

[teoljungberg]

I'm going to 👍 writing warning-free code, we should atleast strive towards that all of the libraries we maintain/write should be warning free. I agree that it's painful with running with warnings on all of the time, so I usually warning-check my code every now and then like this:

for file in $(find {app,spec,lib} -type f -iname "*.rb"); do
  ruby -wc $file
done

Is hounds list of warnings complete, or do we need to do some research to make it so?

[mike-burns]

I don't see value in this guideline. The actual guide should be e.g. "avoid circular dependencies because it can cause loading issues" or "initialize instance variables before using them to avoid nil errors". Avoiding warnings for the sake of avoiding warnings seems like begging the question; I'd rather we understood the ways in which Ruby code can go wrong and were conscious of those.

[mcmire]

Hmm on second thought I actually agree with @mike-burns here. Seems like it would definitely be more useful to include guides that are more specific.

[croaky]

I don't see value in this guideline. The actual guide should be e.g. "avoid circular dependencies because it can cause loading issues" or "initialize instance variables before using them to avoid nil errors".

That's what I did first in #280 but we ended up here. If there's a strong feeling for that one, I'll open that up and merge it but I don't feel strongly enough about this either way to manage the discussion so I'm going to close this one without merging.

[mcmire]

Lol. Okay. We are definitely silly sometimes.