Use grunt-jscs over deprecated grunt-jscs-checker

[ntwb]

The NPM package grunt-jscs-checker was renamed grunt-jscs, thus grunt-jscs-checker is now deprecated.

See jscs-dev/grunt-jscs#53

Switching to grunt-jscs v1.2.0 over grunt-jscs-checker v0.8.1 results in ~126 errors, all the errors are for spaces around square brackets e.g.

Missing space after opening bracket at src/queue.js :
    94 |    jQuery._queueHooks( this, type );
    95 |
    96 |    if ( type === "fx" && queue[0] !== "inprogress" ) {
----------------------------------------^
    97 |     jQuery.dequeue( this, type );
    98 |    }
Missing space before closing bracket at src/queue.js :
    94 |    jQuery._queueHooks( this, type );
    95 |
    96 |    if ( type === "fx" && queue[0] !== "inprogress" ) {
-----------------------------------------^

Rather than patching each of these files at this stage (happy to if this is in fact the desired coding standard) though I thought I should check if the current rules based in the jQuery presets are preseumed to be accurate in JSCS's jQuery presets file https://github.com/jscs-dev/node-jscs/blob/master/presets/jquery.json, in particular both of these settings:

• "requireSpacesInsideObjectBrackets": "all", - JSCS Docs disallowSpacesInsideObjectBrackets / requireSpacesInsideObjectBrackets
• "requireSpacesInsideArrayBrackets": "all", - JSCS Docs disallowSpacesInsideArrayBrackets / requireSpacesInsideArrayBrackets

[ntwb]

The Travis CI build fails due to the errors I've outlined above.

[mgol]

Thanks for the PR! Those rules are accurate, it's just that we have a lot of old code that doesn't adhere to our code style standards. It seems to be a quite amount of work to correct all these style violations, that's mostly why it hasn't been done yet.

I think we'd be happy if someone was willing to correct those violations. :) But I'll defer to @markelog here, who's one of JSCS maintainers.

[markelog]

Yeah, we specifically do not update jscs until it starts find all possible inconsitencies so we could fix them all in one swift motion.

Thank you for the pull though!

[ntwb]

The 64 whitespace changes ( 2 per rule) are here if your interested for reference.

Thanks for the feedback y'all, I think I've got my head around JSCS enough now, wanted to play in a real repo before starting on adding support for JSCS to WordPress' develop repo. 😖

[dmethvin]

@markelog we could land the whitespace changes from @NWTB if he's got them ready. We'd also need a similar PR for the compat branch.

[markelog]

@dmethvin totally
@ntwb could send us a PR?

[ntwb]

I can indeed, I'll take a look at said compat branch also.

@scottgonzalez comment here:

Today, it's safe to use JSCS to catch all the style-related rules that were dropped from JSHint, so it's safe to upgrade both. There is no requirement today other than being able to programmatically enforce the JSHint options listed in the style guide, even if there's a different tool doing that enforcement (such as JSCS + JSHint).

Thus the question here is, based on the above comment and per the original pull request it should be fine to now bump the deprecated grunt-jscs-checker v0.8.1 to grunt-jscs v1.2.0? (i.e. I'll include the original change in an updated pull request)

[scottgonzalez]

I'll let someone that maintains this repo give the definitive answer, but from my side the answer is yes.

[markelog]

Code-style updates are tedious so if you want it to do it for us, then hooray! :-).

However, jscs dependency update is another story.

It seems we soon update the guide - jquery/contribute.jquery.org#104 by simplifying it. After that, jscs could fully support it. Since core uses fixed dependencies we would need to do another commit bump in the week or so. So i would wait until that time and do it in one action.

[ntwb]

I'll wait until your team has made the decisions linked above, I see no point in spending time here creating and submitting a pull request with white space changes if your about to change those recommendations.

I also don't see the point in using an out of date grunt JSCS tool (grunt-jscs-chqecker) that is 4 major versions behind the current release (grunt-jscs) when the only changes I've seen are improvements that are to paraphrase Scott above "programmatically enforce the JSHint options listed in the style guide" and is an overall improvement and not a regression.

[markelog]

I'll wait until your team has made the decisions linked above, I see no point in spending time here creating and submitting a pull request with white space changes if your about to change those recommendation

Discussion about the code-style changes are not conflicted with https://github.com/ntwb/jquery/commit/f5c217beaf43614609c0b5d21d8cd0c630bcc3ba, therefore you PR will be accepted if you choose to submit it.

I also don't see the point in using an out of date grunt JSCS tool

See above of why i think we should wait until the next version of jscs.

[markelog]

"programmatically enforce the JSHint options listed in the style guide"

Right, those already enforced by currently used version

[ntwb]

Pull requested submitted with JSCS code-style updates.

If you don't want to update to use grunt-jscs that is your decision, I'll only reiterate my point in that the only thing I see is you are not benefiting from the upstream JSCS dependancy and code improvements included in versions 1.x, 1.1.x and 1.2.x of grunt-jscs https://github.com/jscs-dev/grunt-jscs/releases where testing with this jQuery repo that uses JSCS only reveals improvements and zero regressions.

Edit: Without updating to grunt-jscs v1.2.0 then the above pull request would never exist as these changes are not currently detected by existing JSHint or JSCS tests.

[markelog]

https://github.com/jscs-dev/grunt-jscs/releases

Yeah, i precisely know what changed, since i checked sizzle and core repos before i maid those releases.

There is really no hurry to update jscs until it could fully support jquery code-style, after that we could this update across all projects.

But of course, this is just little guy opinion :-), so if we all want to update it now and do the same again soon i'm all for it.

[ntwb]

There is really no hurry to update jscs until it could fully support jquery code-style, after that we could this update across all projects.

I disagree with this statement, there are improvements available now without regressions.

[mgol]

FWIW, I agree with @ntwb that since he already has a patch fixing white space issues it'd be better to update JSCS to a newer version so that we're sure we won't introduce newer ones (I know I've already done it in one commit!).

[markelog]

My point is, we would need to update jscs soon, so why not wait a bit and not make extra commit.

[mgol]

My point is, we would need to update jscs soon, so why not wait a bit and not make extra commit.

I see your point, it's just until everything is implemented you never know how soon it is. :) I'm a little afraid about the Node situation where "soon" for 0.12 started at the end of 2013. Since we already have a patch that makes it compatible with latest JSCS, I'm not too worried about bumping JSCS soon again. Unless you have a strong opinion on that. :)

[ntwb]

The point is my pull request uses grunt-jscs v1.2.0, it does not use grunt-jscs-chqecker v0.8.1, if I use grunt-jscs-checker then there is no code changes for a pull request.

Contributing to jQuery should not be this hard, I'm done here, work it out amongst yourselves.