WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 months ago

Last modified 5 months ago

#28543 closed enhancement (wontfix)

Allow stricter JSHint checks for core files

Reported by: MattyRob Owned by:
Milestone: Priority: normal
Severity: trivial Version: 3.7
Component: Build/Test Tools Keywords:
Focuses: javascript Cc:
PR Number:

Description

http://make.wordpress.org/core/2013/11/13/finding-fixing-javascript-errors-with-jshint/

Many JSHint issues were fixed in core JavaScript files for the release of WordPress 3.8 (if I recall correctly). The above post makes refetene to “curly” and “eqeqeq” being ignored for now but would be revisited in the future.

I think therefore it makes sense to implement a stricter set of JSHint checks for core files. This could be done by simply making the current jshint:core task striker or a new jshint:corestrict could be introduced as per the attached patch.

Attachments (1)

28543.diff (2.7 KB) - added by MattyRob 5 years ago.

Download all attachments as: .zip

Change History (10)

@MattyRob
5 years ago

#1 @MattyRob
5 years ago

  • Keywords has-patch added
  • Severity changed from normal to trivial

#2 follow-up: @jorbin
5 years ago

  • Keywords close added

I think the route we should take here is to fix these two issues individually rather than introduce a new temporary grunt task that won't pass. Introducing this task would cause the general jshint task to fail every single time until both curly and eqeqeq are fixed, and once they are fixed we wouldn't need this task.

We should handle turning off the curly and eqeqeq separately. eqeqeq is going to be much harder, but curly is something we can definitely do. I think we should close this ticket and open separate tickets for each of those two.

#3 in reply to: ↑ 2 @MattyRob
5 years ago

Replying to jorbin:

I think the route we should take here is to fix these two issues individually rather than introduce a new temporary grunt task that won't pass. Introducing this task would cause the general jshint task to fail every single time until both curly and eqeqeq are fixed, and once they are fixed we wouldn't need this task.

We should handle turning off the curly and eqeqeq separately. eqeqeq is going to be much harder, but curly is something we can definitely do. I think we should close this ticket and open separate tickets for each of those two.

I'm not sure that I fully understand what you are saying, are you saying that creating a new 'jshint:corestrict' task that can be invoked with a command of "grunt jshint:core strict" that the build tests will fail all the time? Why would this new task be called as part of the automated build tool tests?

Perhaps your suggested way is cleaner by staging the removal of the current exclusions from 'jshint:core' and I did suggest that as an option above.

#4 @MattyRob
5 years ago

  • Keywords dev-feedback added

Just as an update, since my last post I have run the 'core strict' checks and corrected all issues in the 38 affected files. I have't had chance to check as that's probably too much to expect for one set of eyes but I would submit that these may be possible to fix concurrently and in fact - if desired - I can submit a patch or patches for all affected files.

#5 @MattyRob
5 years ago

Related: #28464

A new patch on the above ticket allows specific JSHint tasks to be registered for the build tests thus allowing additional tests to be added without breaking the build tests.

Last edited 5 years ago by SergeyBiryukov (previous) (diff)

#6 @nacin
5 years ago

  • Milestone changed from Awaiting Review to Future Release
  • Version changed from trunk to 3.7

I don't feel strongly about this either way. I'd like to maybe handle curly in 4.1 (I have a script) and start to hack away at eqeqeq.

#7 @wonderboymusic
4 years ago

  • Keywords ongoing added; has-patch close dev-feedback removed

#8 @MattyRob
5 months ago

  • Resolution set to wontfix
  • Status changed from new to closed

I suspect this will be handled via the WordPress Coding Standards project.

Closing this ticket due to lack of traction, likely to be picked up elsewhere and my lack of interest in contributing to WordPress anymore.

#9 @desrosj
5 months ago

  • Keywords ongoing removed
  • Milestone Future Release deleted
Note: See TracTickets for help on using tickets.