Fix Warnings #695

Closed
wants to merge 16 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

tenderlove commented Mar 22, 2013

Hi,

I've read previous pull requests that tried to eliminate warnings. However, I think the Sass codebase contains some great examples on why running with -w is a good thing.

I will enumerate the examples here and link to commits that deal with the issue.

Unused variable warnings

Sass assigns values to variables which are unused. In some cases, the code has no side effects and can be eliminated. I was able to delete code:

Calls to deprecated code

Sass calls methods on URI that will eventually be deleted. I fixed that here.

Redefined methods

Sass defines tests that never run because the methods were redefined. I changed the test name here, here, and could even remove completely duplicated code here.

Uninitialized instance variables

Uninitialized instance variables led me to find an instance variable that is probably misspelled. @throw_err is never set anywhere, and the positive branch of that if statement is never executed in the tests.

Circular requires

Circular requires could possibly lead to a deadlock, so I fixed those.

Shadowed variable warnings

Shadowed variables could refer to variables in the outside scope, so I fixed those here and here.

Setting external encodings

I eliminated a warning around setting the external encoding. Does this mean that Sass requires people to set the external_encoding to UTF-8? I suspect it's possible to eliminate this requirement.

Literals in conditionals

This is probably my most dubious commit. I know team Sass uses literals in conditionals as coding style, but it causes warnings (since it will always evaluate the same way). I changed the literals to constants as a compromise between readability and eliminating warnings.

Duplicate character class warnings

I left these. I think it's a bug in Ruby, so I filed a ticket.

Anyway, all of these are great examples of why I like to run -w on my code. I realize Team Sass may not care for using -w, so I've purposely not squashed these commits in the hope that some (if not all) of them are merged to mainline. Please feel free to cherry pick what you do want, and leave what you don't.

Also, I really really want to say thanks for making the tests super easy to get running. It's not often I can clone a repo and run rake and it does the right thing. THANK YOU! 😄

❤️❤️❤️❤️❤️❤️❤️❤️❤️❤️❤️❤️❤️❤️

Contributor

nex3 commented Mar 22, 2013

You make a pretty compelling case for the utility of some of these warnings. That said, I still don't like the idea of warnings that make the code less readable, like the warnings for !:foo and to some degree uninitialized instance variables (although the @throw_err issue is worrisome).

I'll go through the individual commits and leave review comments. Once those are addressed, I'll merge in everything other than the literal-in-conditional and uninitialized-ivars. I'll fix the @throw_err thing myself.

Thanks for the PR, @tenderlove.

I'd like to have the way these subclasses are required be consistent across all of them. I'd rather see them all required here as they are now, and the back-requires in the individual files be removed.

Owner

tenderlove replied Mar 22, 2013

I can change the requires to the other direction, but I think that files should require what they need. The String class needs the Literal class, so it should require the Literal class, not the other way around.

nex3 replied Mar 22, 2013

I like that philosophy in general, but it doesn't really work out if circular requires aren't allowed. Literal needs Bool, and Bool needs Literal. A semantic use of require there would be circular.

Given that a fully semantic model doesn't work, the "require from superclass" model at least guarantees that the superclass will be defined when the subclass is required.

Owner

tenderlove replied Mar 22, 2013

Ok. I've changed it in d2ee3a6

ged replied Mar 28, 2013

A pattern that retains semantic properties but avoids circular requires:

require 'sass/script/literal' unless defined? Sass::Script::Literal

As per the comment, this can be removed entirely now.

Owner

tenderlove replied Mar 22, 2013

Sounds good, I'll rm this code! :D

There's no need for this, or the :nodoc: comment. Half the methods in Sass::Util are hacks around compatibility-issues.

Owner

tenderlove replied Mar 22, 2013

Sounds good, I'll delete the comment.

Can we cache the parser? Something like $parser ||= URI.const_defined?(:DEFAULT_PARSER) ? URI::DEFAULT_PARSER : URI?

Owner

tenderlove replied Mar 22, 2013

Sure. I'll cache the value.

test_summarized_selectors_with_element

Owner

tenderlove replied Mar 22, 2013

Thanks!

The second one of these can just be test_nested_extender_with_early_child_selector.

Owner

tenderlove replied Mar 22, 2013

OK, I'll fix it.

nex3 commented on 220d5c7 Mar 22, 2013

This is in test code; we set the encoding so that tests that care about that sort of thing will behave consistently in different environments.

Owner

tenderlove replied Mar 22, 2013

Gotcha, I'll remove the comment. Should the tests that require utf8 as the external encoding be specially marked?

nex3 replied Mar 22, 2013

In an ideal world, yeah, probably, but it's often hard to tell which tests rely on it implicitly.

Why this change? Generally we make all the attributes on these classes mutable.

Owner

tenderlove replied Mar 22, 2013

There is a children= method defined later in the file

nex3 replied Mar 22, 2013

Oh, sure.

Contributor

tenderlove commented Mar 22, 2013

You make a pretty compelling case for the utility of some of these warnings. That said, I still don't like the idea of > warnings that make the code less readable, like the warnings for !:foo and to some degree uninitialized instance variables (although the @throw_err issue is worrisome).

I prefer my libraries to run warning free so that app developers are free to make the choice to use -w or not, but I definitely see where you're coming from. -w helps me track down bugs (like some of these) and it's nice to have the option to use it.

I'll go through the individual commits and leave review comments. Once those are addressed, I'll merge in everything other than the literal-in-conditional and uninitialized-ivars. I'll fix the @throw_err thing myself.

Awesome, thank you! 😄

Thanks for the PR, @tenderlove.

No problem, I'm glad to help out. I've pushed a commit that fixes all the stuff you commented on except for the circular require issue. If you feel strongly about that one, then I'll change it as well.

Thanks for your time!

@nex3 nex3 added a commit that referenced this pull request Mar 22, 2013

@nex3 nex3 Merge branch 'warnings'
Closes #695
a3ffe87

nex3 closed this Mar 22, 2013

Contributor

josh commented Apr 8, 2013

❤️

Hopefully I can remove this line from my tests - https://github.com/sstephenson/sprockets/blob/master/test/test_sass.rb#L20-L22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment