Add jscs to check style #1565

hzoo · 2015-03-26T01:43:57Z

I just copied the format from jshint so it will run it in on the same files (and same ignores).

I didn't know what the rules should be (so we can start discussing that now based on the code) so I just picked the google preset as a start (using the jscs autoconfigure option (jscs --autoconfigure src/ lib/ feature-detects/)

One option is to use it as a base or just copy the rules from the preset and add/remove as needed. If there isn't a written style guide or another one to follow we can just look at what's the majority.

Another thing would be how to go about fixing the styles after determining rules.
We can use the autofix setting jscs --auto-fix (only in master branch) to fix most of the whitespace changes and everything else would be manual. I went ahead and did the autofix changes in a seperate commit so the potential changes of using the google preset can be seen. I can remove the 2nd commit or make a new PR to actually fix the changes later.

And then adding it to the grunt test task after all that's fixed? Maybe that should be a separate issue? Right now you can do grunt jscs though after an npm install.

patrickkettner · 2015-04-23T02:26:40Z

@hzoo thank you so much for this!
Would you be able to rebase against master? A single commit for all of these changes would be best.

thanks again!

ryanseddon · 2015-04-23T02:36:40Z

feature-detects/css/remunit.js

@@ -25,7 +25,7 @@ define(['Modernizr', 'createElement'], function( Modernizr, createElement ) {
    try {
      div.style.fontSize = '3rem';
    }
-    catch( er ) {}
+    catch ( er ) {}


Can we add a rule here to make it drop the whitespace inside the parenthesis too?

Sure - I'm just using the google preset as a base/start so yeah feel free for more suggestions in rules to add/remove.

believe that would be "disallowSpacesInsideParentheses": true

looks like google aligns pretty closely with our own style. it generates the least amount of style changes from all of the presets (crockford is 7k haha).

Yeah I think I picked the one with the least errors (forgot)

Yep let's set that to true as we're pretty inconsistent on that front looking at the files.

Interesting.. I don't know why but I had nulled the disallowSpacesInsideParentheses rule in the PR haha.

while we are at it, requireCurlyBraces should be true as well

Ok so with that if (elem === false) return props[i];

would be

if (elem === false) { return props[i]; }

correctamundo

ryanseddon · 2015-04-23T02:42:44Z

Another thing we would want to add is a lint task that runs jshint & jscs much like the task in jQuery

patrickkettner · 2015-04-23T02:43:41Z

@ryanseddon you mean something like grunt.registerTask('lint', ['jshint', 'jscs'])?

patrickkettner · 2015-04-23T02:44:02Z

hah - didn't notice the link. agreed

ryanseddon · 2015-04-23T02:45:31Z

Yes exactly like that and our test task just calls lint

hzoo · 2015-04-23T02:51:41Z

grunt.registerTask('lint', ['jshint', 'jscs'])

Ok so in the current Gruntfile

Modernizr/Gruntfile.js

Line 221 in 9e249d5

grunt.registerTask('default', [ 'jshint', 'build' ]);

There's already the test and default tasks that run jshint. Should I change the references to run lint instead?

patrickkettner · 2015-04-23T02:52:38Z

we would want to change that from jshint to lint, after creating the above mentioned lint task

hzoo · 2015-04-23T02:54:33Z

Gruntfile.js

@@ -49,6 +49,16 @@ module.exports = function( grunt ) {
        ]
      }
    },
+    jscs: {
+      src: [
+        'Gruntfile.js',


These are the same a jshint - any changes needed?

should be good

hzoo · 2015-04-23T03:12:44Z

Autofix is great! Changed the requireCurly's manually since autofix only does whitespace currently (so those shouldn't break anything). Ran gulp lint with no issues. Going to check tomorrow and update if there's any more issues/comments.

patrickkettner · 2015-04-23T03:31:41Z

since this commit modified the Grunt file, saucelabs won't run. I am running them locally just to confirm (I can't imagine it would break anything, but weirder crap has come up previously). Once it goes green I am good to merge 👍

ryanseddon · 2015-04-23T03:32:50Z

.jscsrc

@@ -0,0 +1,9 @@
+{
+	"preset": "google",


Can you make this 2 spaces to match other rc files

patrickkettner · 2015-04-23T03:33:08Z

sall good.

great work, @hzoo!

Add jscs to check style

patrickkettner · 2015-04-23T03:33:41Z

ugh, sorry @ryanseddon. I'll update real quick

ryanseddon · 2015-04-23T03:34:11Z

no problem it's not a huge one

hzoo · 2015-04-23T14:13:47Z

Cool! - I guess editorconfig didn't catch that since I must of copied another file with 4 spaces. There's also a validateIndentation rule although it seems to have some bugs - probably the most difficult rule jscs has.

ryanseddon reviewed Apr 23, 2015
View reviewed changes

hzoo reviewed Apr 23, 2015
View reviewed changes

add jscs to check style, add lint task

5e3f359

hzoo force-pushed the add-jscs branch from c15fd40 to 5e3f359 Compare April 23, 2015 03:01

ryanseddon reviewed Apr 23, 2015
View reviewed changes

.jscsrc

@@ -0,0 +1,9 @@

{

"preset": "google",

Copy link

Member

ryanseddon Apr 23, 2015

Choose a reason for hiding this comment

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

Can you make this 2 spaces to match other rc files

patrickkettner added a commit that referenced this pull request Apr 23, 2015

Merge pull request #1565 from hzoo/add-jscs

3cad259

Add jscs to check style

patrickkettner merged commit 3cad259 into Modernizr:master Apr 23, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add jscs to check style #1565

Add jscs to check style #1565

hzoo commented Mar 26, 2015

patrickkettner commented Apr 23, 2015

ryanseddon Apr 23, 2015

hzoo Apr 23, 2015

patrickkettner Apr 23, 2015

patrickkettner Apr 23, 2015

hzoo Apr 23, 2015

ryanseddon Apr 23, 2015

hzoo Apr 23, 2015

patrickkettner Apr 23, 2015

hzoo Apr 23, 2015

patrickkettner Apr 23, 2015

ryanseddon commented Apr 23, 2015

patrickkettner commented Apr 23, 2015

patrickkettner commented Apr 23, 2015

ryanseddon commented Apr 23, 2015

hzoo commented Apr 23, 2015

patrickkettner commented Apr 23, 2015

hzoo Apr 23, 2015

patrickkettner Apr 23, 2015

hzoo commented Apr 23, 2015

patrickkettner commented Apr 23, 2015

ryanseddon Apr 23, 2015

patrickkettner commented Apr 23, 2015

patrickkettner commented Apr 23, 2015

ryanseddon commented Apr 23, 2015

hzoo commented Apr 23, 2015

Add jscs to check style #1565

Add jscs to check style #1565

Conversation

hzoo commented Mar 26, 2015

patrickkettner commented Apr 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanseddon commented Apr 23, 2015

patrickkettner commented Apr 23, 2015

patrickkettner commented Apr 23, 2015

ryanseddon commented Apr 23, 2015

hzoo commented Apr 23, 2015

patrickkettner commented Apr 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzoo commented Apr 23, 2015

patrickkettner commented Apr 23, 2015

Choose a reason for hiding this comment

patrickkettner commented Apr 23, 2015

patrickkettner commented Apr 23, 2015

ryanseddon commented Apr 23, 2015

hzoo commented Apr 23, 2015