Opened 11 years ago
Closed 11 years 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)
Change History (53)
#2
@
11 years 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.
#3
@
11 years 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.
#4
@
11 years 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
@
11 years ago
Add .jshintrc for qunit and bring existing the unit test file in line with jQuery style guide
#7
in reply to:
↑ 6
@
11 years 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).
#8
@
11 years 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?
#11
@
11 years 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
#12
@
11 years 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
#14
@
11 years 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.
#15
@
11 years 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?
#16
@
11 years 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?
@
11 years ago
Adds "wp-includes/js/tinymce/langs/wp-langs-en.js", which was missed in the last diff.
#17
@
11 years ago
@tollmanz - There are two alternatives that I see:
- We could use the {} globbing pattern a bit, but I think that listing the files out the way you did is more readable.
- 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.
@
11 years ago
Suppress "curly" and "eqeqeq" errors for WP code, remove duplicate "onevar" from gruntfile jshint config, add common libs to jshintrc, and alphabetize jshintrc
#19
@
11 years 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
#23
follow-up:
↓ 25
@
11 years ago
In 25187.15.diff: make it possible to process only one file passed from cli:
$ jslint:core --target=filename.js
#24
@
11 years 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
#25
in reply to:
↑ 23
@
11 years 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!
#27
@
11 years 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 ...
#28
@
11 years ago
+1 on generalizing the filter code. This would be particularly useful when working on unit tests.
#29
@
11 years 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.
25187.diff implements the new files.