WordPress.org

Make WordPress Core

Opened 8 months ago

Closed 5 months ago

#25187 closed enhancement (fixed)

.jshintrc & .jshintignore support

Reported by: carldanley Owned by:
Milestone: 3.8 Priority: normal
Severity: normal Version: 3.8
Component: Build/Test Tools Keywords: dev-feedback needs-testing
Focuses: Cc:

Description

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.

Attachments (19)

25187.diff (2.3 KB) - added by carldanley 8 months ago.
25187.2.diff (3.8 KB) - added by kadamwhite 6 months ago.
.jshintrc adapted from the jQuery project, with appropriate Grunt configuration
25187.3.diff (3.8 KB) - added by kadamwhite 6 months ago.
Added "onevar" to Gruntfile jshint options
25187.4.diff (3.9 KB) - added by kadamwhite 6 months ago.
Add "jQuery" and "wp" as globals
25187.5.diff (1.3 KB) - added by jorbin 6 months ago.
25187.6.diff (4.9 KB) - added by kadamwhite 6 months ago.
Add .jshintrc for qunit and bring existing the unit test file in line with jQuery style guide
25187.7.diff (249 bytes) - added by jorbin 6 months ago.
add ajaxurl to globals
25187.8.diff (280 bytes) - added by jorbin 5 months ago.
25187.9.diff (274 bytes) - added by jorbin 5 months ago.
25187.10.diff (1.4 KB) - added by tollmanz 5 months ago.
Implement jshinting for WP code within "3rd Party Scripts" directories
25187.11.diff (1.5 KB) - added by tollmanz 5 months ago.
Adds "wp-includes/js/tinymce/langs/wp-langs-en.js", which was missed in the last diff.
25187.11.2.diff (1.5 KB) - added by tollmanz 5 months ago.
Whoops! File path was not correct in 25187.11.diff
25187.12.diff (1.3 KB) - added by kadamwhite 5 months ago.
Suppress "curly" and "eqeqeq" errors for WP code, remove duplicate "onevar" from gruntfile jshint config, add common libs to jshintrc, and alphabetize jshintrc
25187.13.diff (1.4 KB) - added by kadamwhite 5 months ago.
Update unit test JS to pass now that "onevar" is true
25187.14.diff (2.8 KB) - added by nacin 5 months ago.
25187.15.diff (831 bytes) - added by azaozz 5 months ago.
25187.16.diff (829 bytes) - added by kadamwhite 5 months ago.
Standardize spacing and make JSHint filter pass eqeqeq
25187.17.diff (2.0 KB) - added by azaozz 5 months ago.
25187.18.diff (3.4 KB) - added by tollmanz 5 months ago.

Download all attachments as: .zip

Change History (53)

carldanley8 months ago

comment:1 carldanley8 months ago

25187.diff implements the new files.

comment:2 jorbin8 months ago

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.

comment:3 carldanley8 months ago

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.

Last edited 8 months ago by carldanley (previous) (diff)

kadamwhite6 months ago

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

kadamwhite6 months ago

Added "onevar" to Gruntfile jshint options

kadamwhite6 months ago

Add "jQuery" and "wp" as globals

comment:4 kadamwhite6 months ago

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

comment:5 nacin6 months ago

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.

jorbin6 months ago

comment:6 follow-up: jorbin6 months ago

the above patch makes the jshint:qunit task pass.

kadamwhite6 months ago

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

comment:7 in reply to: ↑ 6 kadamwhite6 months ago

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).

comment:8 jorbin6 months ago

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?

jorbin6 months ago

add ajaxurl to globals

comment:9 jorbin6 months ago

related: #25754

comment:10 nacin6 months ago

In 25992:

Add jshintrc to qunit.

props jorbin.
see #25187.

jorbin5 months ago

comment:11 jorbin5 months ago

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

comment:12 kadamwhite5 months ago

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

jorbin5 months ago

comment:13 azaozz5 months ago

In 26025:

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

comment:14 tollmanz5 months ago

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.

tollmanz5 months ago

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

comment:15 jorbin5 months ago

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?

comment:16 tollmanz5 months ago

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?

tollmanz5 months ago

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

tollmanz5 months ago

Whoops! File path was not correct in 25187.11.diff

comment:17 jorbin5 months ago

@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.

comment:18 tollmanz5 months ago

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

kadamwhite5 months ago

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

kadamwhite5 months ago

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

nacin5 months ago

comment:19 kadamwhite5 months ago

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

comment:20 nacin5 months ago

In 26040:

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

props kadamwhite.
see #25187.

comment:21 nacin5 months ago

In 26041:

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

props kadamwhite.
see #25187.

comment:22 nacin5 months ago

In 26042:

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

azaozz5 months ago

comment:23 follow-up: azaozz5 months ago

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

$ jslint:core --target=filename.js

kadamwhite5 months ago

Standardize spacing and make JSHint filter pass eqeqeq

comment:24 kadamwhite5 months ago

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

comment:25 in reply to: ↑ 23 adamsilverstein5 months ago

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!

comment:26 azaozz5 months ago

In 26043:

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

comment:27 azaozz5 months ago

We can also take that filter function outside the jshint: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
...
Last edited 5 months ago by azaozz (previous) (diff)

azaozz5 months ago

comment:28 kadamwhite5 months ago

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

comment:29 azaozz5 months ago

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.

tollmanz5 months ago

comment:30 tollmanz5 months ago

Removing this comment as it was in the wrong ticket.

Last edited 5 months ago by tollmanz (previous) (diff)

comment:31 tollmanz5 months ago

Oopsie...wrong ticket. Sorry about that.

comment:32 nacin5 months ago

In 26136:

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

comment:33 nacin5 months ago

In 26209:

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

comment:34 nacin5 months ago

  • Component changed from General to Build Tools
  • Milestone changed from Awaiting Review to 3.8
  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.