Fix Warnings #695
Fix Warnings #695
Conversation
|
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 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 Thanks for the PR, @tenderlove. |
I prefer my libraries to run warning free so that app developers are free to make the choice to use
Awesome, thank you! 馃槃
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! |
|
鉂わ笍 Hopefully I can remove this line from my tests - https://github.com/sstephenson/sprockets/blob/master/test/test_sass.rb#L20-L22 |
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
-wis 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_erris 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
-won 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
rakeand it does the right thing. THANK YOU! 馃槃鉂わ笍鉂わ笍鉂わ笍鉂わ笍鉂わ笍鉂わ笍鉂わ笍鉂わ笍鉂わ笍鉂わ笍鉂わ笍鉂わ笍鉂わ笍鉂わ笍