.jshintrc & .jshintignore support

[carldanley]

Implement jshint configuration standards so everyone is on the same page. See my notes in this gist: ​https://gist.github.com/carldanley/6384499. Feel free to open discussion on any config option. Thinking we can patch that gist until we feel really confident about it and then I'll submit a formal patch when ready.

[carldanley]

25187.diff​ implements the new files.

[jorbin]

Do we need to be declaring every option in jshint? I think it would be easier to read the jshint file if we only included the options where we disagreed with the default.

[carldanley]

Definitely not. I put them there for starters so everyone relevant can view in the event they're unsure of what each option is. We can remove the defaults before we patch; totally agree with that.

[kadamwhite]

.jshintrc adapted from the jQuery project, with appropriate Grunt configuration

[kadamwhite]

Added "onevar" to Gruntfile jshint options

[kadamwhite]

Add "jQuery" and "wp" as globals

[kadamwhite]

Added .jshintrc adapted from the jQuery project, with these modifications:

• Prefer single quotes to double
• Remove "boss" option
• Add "jQuery" and "wp" as globals

We do not enforce onevar in .jshintrc, due to the number of files in which this is an issue. It can be added file-by-file as they get fixed, and rolled into the global config down the road.

gnarf37 & I did this work at the wcbos dev day after discussion with mattweibe

[nacin]

In 25960:

Add a .jshintrc file and associated jshint grunt task.

This .jshintrc is adopted from the jQuery project, with some basic modifications (such as single quotes instead of double quotes). This pretty closely follows our current JS standards and rather closely resembles our PHP standards, especially the love of whitespace. The major changes are enforcing === and always using braces for if statements.

props kadamwhite, gnarf37, with mattwiebe and carldanley.
see #25187.

[jorbin]

the above patch makes the jshint:qunit task pass.

[kadamwhite]

Add .jshintrc for qunit and bring existing the unit test file in line with jQuery style guide

[kadamwhite]

Replying to jorbin:

the above patch makes the jshint:qunit task pass.

Good catch! Corey actually worked on this a bit yesterday, too, but in my excitement I left Corey's QUnit .jshintrc out from the original patch. I've uploaded 25187.6.diff, which contains exactly what jorbin is suggesting and also brings the test file in line with the jQuery style guide (per our conversation with @mattwiebe, @nacin, @gnarf37, and @tommcfarlin about formally adopting most of that style guide).

[jorbin]

I do not agree with the style being used in the for loops on 25187.6. I think that they make the for loops harder to read and should not be the style we aim for. Resetting i for use in another loop is not obvious and we should avoid using the variable name length and instead be more obvious as to which length we are talking about. (yes, I may have been the one to originally use that name. I'm not above saying I did it wrong).

Can you share the rest of this meat space conversation?

[jorbin]

add ajaxurl to globals

[jorbin]

related: #25754

[nacin]

In 25992:

Add jshintrc to qunit.

props jorbin.
see #25187.

[jorbin]

Above patch adds es3 and onevar as agreed upon in the irc chat for JS standards.

​https://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2013-11-06&sort=asc#m722215

[kadamwhite]

Once onevar and es3 are added, we should probably close this and move on. Per the IRC log linked above, individual files' jshint errors will be addressed incrementally in the manner being used by the inline docs effort

[azaozz]

In 26025:

Add es3 and onevar to the JSHint configuration as discussed in IRC. Props jorbin, see #25187.

[tollmanz]

I just audited the JS files in WordPress in order to prepare a list of files that we need to cleanup in order for jshint to pass. In this process, I found that a number of files that are WordPress code, not 3rd party code, are buried in the "3rd-Party Scripts" excludes. These all need to be added to the jshint task:

# Could not be positively identified as 3rd party
wp-includes/js/jquery/jquery.table-hotkeys.js
 
# WP specific tinyMCE plugins
wp-includes/js/tinymce/plugins/wordpress/editor_plugin_src.js
wp-includes/js/tinymce/plugins/wpdialogs/editor_plugin_src.js
wp-includes/js/tinymce/plugins/wpdialogs/js/popup.js
wp-includes/js/tinymce/plugins/wpdialogs/js/wpdialog.js
wp-includes/js/tinymce/plugins/wpeditimage/editor_plugin_src.js
wp-includes/js/tinymce/plugins/wpeditimage/js/editimage.js
wp-includes/js/tinymce/plugins/wpfullscreen/editor_plugin_src.js
wp-includes/js/tinymce/plugins/wpgallery/editor_plugin_src.js
wp-includes/js/tinymce/plugins/wplink/editor_plugin_src.js
wp-includes/js/tinymce/plugins/wpview/editor_plugin_src.js
 
# Other
wp-includes/js/tinymce/wp-tinymce-schema.js
wp-includes/js/mediaelement/wp-mediaelement.js
wp-includes/js/plupload/wp-plupload.js

A thorough accounting of jshint status can be found here: ​https://gist.github.com/tollmanz/7348654. @kadamwhite should be posting this to a Make blog tomorrow.

I found that reinstating these individual files after the directories have been excluded works the charm for getting them jshinted. If there is a better way to do this, I am all ears.

[tollmanz]

Implement jshinting for WP code within "3rd Party Scripts" directories

[jorbin]

Based on #7643, I'm pretty sure wp-includes/js/jquery/jquery.table-hotkeys.js isn't third party.

@nbachiyski - Any chance you remember from 5+ years ago?

[tollmanz]

I absolutely couldn't find that ticket. Yes...that seems to be WP code based on that ticket. If it's not, there is a good chance that we are violating some license ;)

@jorbin - Do the changes to Gruntfile.js look good? Know of any other way to do that?

[tollmanz]

Adds "wp-includes/js/tinymce/langs/wp-langs-en.js", which was missed in the last diff.

[tollmanz]

Whoops! File path was not correct in 25187.11.diff

[jorbin]

@tollmanz - There are two alternatives that I see:

1. We could use the {} ​globbing pattern a bit, but I think that listing the files out the way you did is more readable.
2. We can use the method that ​jQuery uses of grouping files and then mapping them, but I think this only makes sense if we are going to reuse the list for other tasks.

Overall, I think your method makes the most sense for now.

[tollmanz]

Thanks @jorbin! I agree. This method seems to be the most sensible.

[kadamwhite]

Suppress "curly" and "eqeqeq" errors for WP code, remove duplicate "onevar" from gruntfile jshint config, add common libs to jshintrc, and alphabetize jshintrc

[kadamwhite]

Update unit test JS to pass now that "onevar" is true

[kadamwhite]

Added two patches: One to adjust our global JSHint options,

• Add Backbone and _ as universally permitted global variables
• Set "curly" and "eqeqeq" to FALSE within the Gruntfile's jshint configuration for wp-admin/ and wp-includes/, to aid in addressing the code smell first
• Alphabetize jshintrc options for readability

and the other to make the unit test JSHint run pass now that "onevar" is true

[nacin]

In 26040:

Use only one var per scope in our password-strength-meter tests.

props kadamwhite.
see #25187.

[nacin]

In 26041:

Update jshint grunt task to exclude eqeqeq and curly for now, until we've resolved other code smells.

props kadamwhite.
see #25187.

[nacin]

In 26042:

Ignore wp-langs-en.js during linting, per azaozz. see #25187.

[azaozz]

In 25187.15.diff: make it possible to process only one file passed from cli:

$ jslint:core --target=filename.js

[kadamwhite]

Standardize spacing and make JSHint filter pass eqeqeq

[kadamwhite]

In 25187.16.diff, made these adjustments to azaozz's jshint filter:

• Fixed a jshint error within the jshint grunt task filter method :D
• Made the spacing consistent
• Changed "target" to "file," since that's what we mean

Can be run by

grunt jshint:core --target=xfn.js

[adamsilverstein]

Replying to azaozz:

In 25187.15.diff: make it possible to process only one file passed from cli:

$ jslint:core --target=filename.js

brilliant! this will really help getting thru the files one by one, thanks!

[azaozz]

In 26043:

Grunt: limit JSHint's run to a single specified file. Run with grunt jshint:core --file=filename.js. Props kadamwhite, see #25187.

[azaozz]

We can also take that filter function outside the core task and add it to all of jshint. Perhaps a bit easier to run: grunt jshint --file=about.js, but will output a few more lines:

$ grunt jshint --file=about.js
Running "jshint:grunt" (jshint) task
>> 0 files linted. Please check your ignored files.

Running "jshint:tests" (jshint) task
>> 0 files linted. Please check your ignored files.

Running "jshint:core" (jshint) task
Linting src/wp-admin/js/about.js ...ERROR
...

[kadamwhite]

+1 on generalizing the filter code. This would be particularly useful when working on unit tests.

[azaozz]

Yeah, as the filtering is done while grunt selects files to process, this function (with some tweaks) can be used in any task to limit to one file or directory or to exclude a file or directory. Can also be made to accept multiple files/directories from the cli if needed.

[tollmanz]

Removing this comment as it was in the wrong ticket.

[tollmanz]

Oopsie...wrong ticket. Sorry about that.

[nacin]

In 26136:

Remove smarttabs from .jshintrc per IRC discussion. see #25187.

[nacin]

In 26209:

JSHint fixes for word-count.js and wp-mediaelement.js. see #25187.