-
Notifications
You must be signed in to change notification settings - Fork 96
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
Upgrade JSHint to 2.5.4 #177
Conversation
Previously we vendored + patched a copy of JSHint each time we needed to upgrade. This commit takes a plain copy of JSHint and adapts PreCommit to use JSHint out of the box. This should make future JSHint upgrades much easier :)
looks good to me ;) |
@@ -5,7 +5,7 @@ module Checks | |||
class Jshint < Js | |||
|
|||
def js_config | |||
if config_file | |||
@js_config ||= if config_file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you use this js_config
outside this class? Can it be made private?
+1, looks legit, left some minor notes. The build's broken though? |
When appending JavaScript it is considered best practice to include an initial semi-colon to ensure the concatenated content does not collide.
💇 updated with suggestions I'm not sure what to do about the JRuby test failures. Should we drop JRuby from Travis for the time being? |
+1. Maybe mark the jruby build as optional in the travis matrix? I haven't looked at the failure myself, so take that as a light suggestion. |
|
jruby issue jruby/jruby#1945 - will be fixed in |
+1 |
Previously we vendored + patched a copy of
JSHint
each time we needed to upgrade. This commit takes a plain copy ofJSHint
and adaptsPreCommit
to useJSHint
out of the box.This should make future
JSHint
upgrades much easier :)@shajith @mpapis @mwerner
Closes #157