WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#28464 closed enhancement (fixed)

grunt jshint for plugins and plugin developers

Reported by: MattyRob Owned by: wonderboymusic
Milestone: 4.0 Priority: normal
Severity: normal Version: 4.0
Component: Build/Test Tools Keywords: has-patch commit
Focuses: Cc:
PR Number:

Description

Grunt jshint can be used on core files and theme files (with the latter seeming limited to twenty fourteen at the moment).

I've extended my local Gruntfile.js to allow the plugins folder to be scanned and also implemented a 'folder' filter so a plugin folder name can be specified to be scanned.

This allows plugin developers easier access to JSHint and supports good coding standards. Is it worth adding to core?

Attachments (4)

28464.diff (1.0 KB) - added by MattyRob 5 years ago.
28464v2.diff (953 bytes) - added by MattyRob 5 years ago.
28464v3.diff (1.0 KB) - added by MattyRob 5 years ago.
28464.corejs.diff (1.2 KB) - added by netweb 5 years ago.

Download all attachments as: .zip

Change History (19)

@MattyRob
5 years ago

#1 @MattyRob
5 years ago

  • Keywords has-patch added

#2 follow-up: @jorbin
5 years ago

  • Milestone changed from Awaiting Review to 4.0

This looks like a good addition to me and one that will help plugin developers.

My only feedback is that I think the inline docs are a bit repetitive. I think the first line can have Optionally added to the start of it and it would be clearer.

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

Replying to jorbin:

This looks like a good addition to me and one that will help plugin developers.

My only feedback is that I think the inline docs are a bit repetitive. I think the first line can have Optionally added to the start of it and it would be clearer.

That's a good point - I'd just copied and pasted from the 'core' filename filter and through my edits made them the same! Update patch on the way.

@MattyRob
5 years ago

#4 @jorbin
5 years ago

  • Keywords commit added

Looks good to me.

This ticket was mentioned in IRC in #wordpress-dev by jorbin. View the logs.


5 years ago

#6 @wonderboymusic
5 years ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from new to closed

In 28798:

Add a grunt jshint:plugins task.

Props MattyRob.
Fixes #28464.

#7 @nacin
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Let's call this dir or directory in lieu of folder.

#8 @MattyRob
5 years ago

As requested, use dir instead of folder. See new attached patch.

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

@MattyRob
5 years ago

#9 @netweb
5 years ago

Two thoughts:

1) This task will only run JSHint on plugins being developed in \src\wp-content\plugins

  • If a plugin developer needs, and I believe that they should, they should be running WordPress trunk from /build via the develop.svn repo or they should use core.svn to develop their plugin.
  • Developing plugins by loading WordPress via \src\wp-content\plugins of the develop.svn repo excludes RTL CSS or admin themes CSS (Midnight, Ocean etc) and I think we should be encouraging plugin authors to test their plugins for compatibility with these core features.

2) The jshint:plugin task is now included by default in grunt precommit and grunt travis:js tasks

  • It does not cause an error running either task though a warning is displayed (see image below)
  • If not we should register a new task eg. jshint:corejs to call from grunt precommit and grunt travis:js so jshint:plugin is not included. eg 28464.corejs.diff
  • https://i.cloudup.com/TYF2875QEj.png

@netweb
5 years ago

#10 @wonderboymusic
5 years ago

In 28812:

Use dir instead of folder in the language for jshint:plugins

Props MattyRob.
See #28464.

#11 follow-up: @helen
5 years ago

grunt precommit needs to not run this, please.

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

#12 in reply to: ↑ 11 ; follow-up: @MattyRob
5 years ago

Replying to helen:

grunt precommit needs to not run this, please.

Isn't that covered in the patch submitted by netweb above?

#13 in reply to: ↑ 12 @netweb
5 years ago

Replying to MattyRob:

Replying to helen:

grunt precommit needs to not run this, please.

Isn't that covered in the patch submitted by netweb above?

Yes, 28464.corejs.diff​ excludes jshint:plugins from both grunt precommit and grunt travis:js tasks.

I'm not overly excited about the naming in that patch, it add's a task jshint:corejs because we already have jshint:core, I was thinking of switching the names of each of those tasks and there are both back compat pro's and con's for either. Happy for any other ideas or methods to implement this.

#14 @helen
5 years ago

Missed that part of the comment, carry on :)

#15 @wonderboymusic
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 28873:

Exclude jshint:plugins from both grunt precommit and grunt travis:js.

Props netweb.
Fixes #28464.

Note: See TracTickets for help on using tickets.