Streamline WPCS in file documentation. #651

jrfnl · 2016-08-01T10:18:49Z

Ok, so this is another quite big one, though it does not contain any pertinent functional changes.

This PR lets the WPCS code base pass the docs sniffs as currently defined.
Additionally, I've tried to make the docs more consistent and removed outdated tags and redundancy.

Overview of changes:

Functional:

Fixed some - unintentional - parse errors in the .inc test files.

File and class doc blocks:

Removed @category tags as per the recommendation in https://www.phpdoc.org/docs/latest/references/phpdoc/tags/category.html
Adjusted the @package tags to reflect a logical hierarchy as per the recommendation in https://www.phpdoc.org/docs/latest/references/phpdoc/tags/package.html
Added @license tags to all file level doc blocks as per the recommendation in https://www.phpdoc.org/docs/latest/references/phpdoc/tags/license.html
Added @since tags to all class level doc blocks based on Git history of the classes. The version mentioned in the @since tag is based on the name of the first released WPCS version in which the class was contained as per https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/releases. For classes which have been in WPCS since it got taken over from John Godley, I've used tag 0.1.
For pertinent changes to a class, I've added a secondary @since tag to document the changes as per the WP core doc standard.
Verified all @author tags and added/corrected some where needed. Removed additional @author tags as per the current docs standard only one @author tag allowed per doc block.
Added @see tags to any deprecated classes as per WP doc standard.
Removed old/copied over tags which were not pertinent and incorrect tags.

Property, method and inline documentation:

Removed a few superfluous @since tags for methods and properties which were part of the class at the point of first release.
Where methods or properties were added at a later point in time than the first official WPCS release the class was contained in, added @since tags to these methods and properties.
Fixed any notices about missing documentation, though in some places with quite rudimentary information. Further improvement/review by the class authors advised.
Fixed capitalization and punctuation in a number of places.
Removed superfluous documentation - this mainly applies to the test files where the explanation of what should be added to the methods was contained in nearly every method doc block. Simplified this by improving the @return tag info.

Test files good/bad documentation:

Add capitalization and punctuation to the comments in the .inc test files (lead by example).

General:

Fixed a few spelling errors.

The file level doc blocks are now the same everywhere. I realize that is not how it's supposed to be, but they don't really add much value anyway when the class doc blocks are good (which they should be now) and at the very least, the file doc blocks are now consistent instead of a mish-mash of copied over info.

Feedback welcome.

I strongly advise for the commits be squashed when merging this, they are just there to show the different iterations and to make reviewing of the changes easier.

P.S.: this PR includes all the necessary @since tags for the 0.10.0 release 😎

GaryJones · 2016-08-01T18:37:23Z

WordPress/AbstractClassRestrictionsSniff.php

@@ -51,8 +53,9 @@
 	 * Documented here for clarity. Not (re)defined as it is already defined in the parent class.
 	 *
 	 * @return array
+	 *
+	abstract public function getGroups();


Something wrong here?

What do you mean ?

"Not (re)defined as it is already defined in the parent class."

Comments style wise if I close the doc block and use a // comment, I get a notice that inline comments need a blank line before them, so this way, the abstract method is still documented in the child abstract class and passes the doc checks.

There may be better ways to do this, but I couldn't think of any. Advice welcome ;-)

JDGrimes · 2016-08-03T14:46:42Z

I think the @package tags should be something different. PHP\CodeSniffer\WordPress-Coding-Standards seems more like a classification of the project as a whole, not the files within the project. I guess the documentation on the @package tag is really unclear on this though. Worth noting:

If, across the board, both logical and functional subdivisions are equal is it NOT RECOMMENDED to use the @package tag, to prevent maintenance overhead.

Since every @package tag is going to be identical right now, it would be better just to leave them out IMO.

JDGrimes · 2016-08-03T15:14:03Z

I'm not sure about including the @license and @link tags in every single file-level docblock. It seems like something that should be in one place, not repeated everywhere. But maybe it is just the DRY side of my brain going nuts. If WET will be helpful to people, then maybe it's OK...

JDGrimes · 2016-08-03T15:20:24Z

Anyone else for just removing all @author tags? Do they really make sense for a project like this where there will be multiple contributors to many sniffs?

GaryJones · 2016-08-03T20:18:55Z

I've all for removing @author tags. Once someone significantly edits a sniff, the existing @author tag becomes incomplete, which means adding more tags. I'd rather let the git blame show attribution, since it that shows authorship per line, not per sniff.

I would say to keep @license on each file-level docblock though - if code gets taken out of context, then the ability to potentially re-use it is there with the file.

@link tags on file-level docblocks should be removed - they aren't valid AFAIK.

@package would be something like WPCS\WordPressCodingStandards IMO. It's possible we could separate out something, or have a new PHP package, in which case, that would have a @package of WPCS\SomeOtherPackage.

JDGrimes · 2016-08-03T21:52:28Z

I'll agree with @GaryJones to keep the @license tags per-file. Also, WPCS\WordPressCodingStandards sounds like a reasonable package name.

JDGrimes · 2016-08-03T21:58:47Z

Removed a few superfluous @since tags for methods and properties which were part of the class at the point of first release.

Does anybody else have thoughts on this? I always include @since tags even if the property/method was in the class from day one. Better to be explicit IMO.

GaryJones · 2016-08-04T07:56:01Z

Does anybody else have thoughts on this? I always include @SInCE tags even if the property/method was in the class from day one. Better to be explicit IMO.

I include them from day one too. If we ever run WPParser / phpDocumentor / APIGen or similar tool, then having the method have explicit @since tags is going to make the generated documentation more complete.

jrfnl · 2016-08-04T18:15:53Z

Thanks for having a look at this.

Regarding the @package tag:
I'm obviously of the opinion that PHP\CodeSniffer\WordPress-Coding-Standards would be good, but maybe I should explain that a bit. This is a package build onto PHPCS, PHPCS uses PHP_CodeSniffer as a @package tag which is an outdated notation for PHP\CodeSniffer. As the @package tag is for annotating a logical subdivision and WPCS is dependent on PHPCS and for use with PHPCS, it seemed the logical thing to do to build onto their @package tag and call it PHP\CodeSniffer\WordPress-Coding-Standards.

Calling it WPCS\WordPressCodingStandards removes any relation to PHPCS which seems counter productive to me.

PHP\CodeSniffer\WordPress-Coding-Standards or PHP\CodeSniffer\WordPressCodingStandards could both be used and I don't have a preference.

As the @package tag for file level applies only to global functions, constants, variables, includes and requires, I'd be quite happy to remove it from the file level and just leave it at the class level.

Regarding the @license and @link file-level tags:
The @link to the handbook pages can be removed IMHO.
The @link to the project GH page and @license should remain in my opinion, for the same reasons @GaryJones mentioned. If a file is encountered at some point out of context, at least the licensing is clear and the source to update the file can be found easily.
Whether the link should use the tag is another matter, we could also just have it as plain text, but that's another discussion.

Regarding the @author tags:
I'm quite happy to remove them.
I updated them as a lot of them were wrong, i.e. copy/paste, forgot to change. If they're there, they should at least be correct.

Regarding the @since tags:
Let's not fool ourselves here: only the methods/properties in the Nonce sniff had them. Anywhere else they were only applied to methods/properties added after the class was first released (if at all and mostly they weren't).
Any good parser will automatically fall back to the higher level class @since tag for any @since tags missing in child elements.

If anyone wants to add them to all methods/properties, feel free, but I honestly don't think it's necessary. At the very least it'll be easier now as I've already done the research of when things were added.

@category

Follow the standard as described in https://www.phpdoc.org/docs/latest/references/phpdoc/tags/package.html The @category tag is considered deprecated. A hierarchical @Package tag should be used instead. Ref: https://www.phpdoc.org/docs/latest/references/phpdoc/tags/category.html

Capitalization, punctuation and some spelling.

…ght type hint. But only if it doesn't conflict with upstream function signatures.

@SInCE

Based on Git file history: * Add @SInCE tags to all classes. * Add @SInCE tags to properties and methods if they weren't included from the start. * Verify/Fix a number of @author tags. * Remove old/copied over/incorrect PHPCS tags. * Where necessary improved/corrected the class description. Also: * Consistent tag order in class doc blocks. * Fix tag alignment. * Remove redundant explanation in unit test class doc blocks. * Sync the description line of the unit test class doc blocks.

Includes information on when the sniff was last synced with the upstream sniff if applicable and available.

… doc blocks. License tag as per https://www.phpdoc.org/docs/latest/references/phpdoc/tags/license.html

jrfnl · 2016-08-04T18:21:14Z

(Rebased for merge conflict)

GaryJones · 2016-08-04T19:56:36Z

This is a package build onto PHPCS, PHPCS uses PHP_CodeSniffer as a @Package tag which is an outdated notation for PHP\CodeSniffer

I disagree that PHP_CodeSniffer => PHP\CodeSniffer. By your logic, that would imply that PHPCS is dependent on (and only on) the PHP "package".

v3 of PHPCS doesn't have @package tags either, but their top level namespace is PHP_CodeSniffer, without the vendor (which they can get away with, being so popular).

As the @Package tag is for annotating a logical subdivision and WPCS is dependent on PHPCS and for use with PHPCS

By that logic, every WP plugin should therefore be given a @package tag starting with WordPress\ then? That would be ridiculous.

Until such time as WPCS is included as a standard inside the PHPCS package, then it doesn't make sense to claim to be a sub-part of their package, just because it's currently our only dependency.

The composer package that can be grabbed from this repo, is independently authored, so our top level vendor and package name would be WPCS\WordPressCodingStandards or a very similar variation IMO.

Any good parser will automatically fall back to the higher level class @SInCE tag for any @SInCE tags missing in child elements.

Although it seems like a logical approach, I see no recommendation for that in the PSR-5 draft or remember seeing it in previous suggestions.

jrfnl · 2016-08-04T21:00:36Z

As PHPCS is still distributed through PEAR as well, their namespace is determined by that.

Also:

Note: phpDocumentor also allows the underscore (_) and dot (.) as separator for compatibility with existing projects. Despite this the backslash is RECOMMENDED as separator.

Ref: https://www.phpdoc.org/docs/latest/references/phpdoc/tags/package.html

By that logic, every WP plugin should therefore be given a @Package tag starting with WordPress\ then?

Actually, yes, I think that would make a lot of sense to do.

JDGrimes · 2016-08-04T21:20:16Z

By that logic, every WP plugin should therefore be given a @Package tag starting with WordPress\ then? That would be ridiculous.

I agree, I've never interpreted it this way. And take a look at what it says in the WordPress inline docs standards:

@package Tag in Plugins and Themes (bundled themes excluded)

Third-party plugin and theme authors must not use @package WordPress in their plugins or themes. The @package name for plugins should be the plugin name; for themes, it should be the theme name, spaced with underscores: Twenty_Fifteen. [emphasis in original]

JDGrimes · 2016-08-04T21:22:44Z

Regarding the @SInCE tags:
[...]
If anyone wants to add them to all methods/properties, feel free, but I honestly don't think it's necessary.

Not so much worried about that, though it would be ideal (IMHO). I was just seeing you removing some, and that's what tipped me off. Let's at least not do that.

jrfnl · 2016-08-04T21:52:48Z

And take a look at what it says in the WordPress inline docs standards:

Yeah, well, WP has kind of a habit of fool hardy doing the opposite of what the wider PHP community agrees upon, so that doesn't surprise me.... To be honest, it sounds more like a brand protection line coming from Automattic than any attempt to comply with real standards.

jrfnl · 2016-08-04T21:54:46Z

Not so much worried about that, though it would be ideal (IMHO). I was just seeing you removing some, and that's what tipped me off. Let's at least not do that.

I don't mind reverting that bit. When I was working on it, I chose to let consistency prevail rather than have small exceptions littered all over the place.

jrfnl · 2016-08-09T13:16:15Z

I've added two new commits based on the feedback which contain the following changes:

Removed the @Package tag at the file level doc block
Removed the @link tag to the handbook at the file level doc block
Removed the @author tags
Re-instate previously removed @SInCE tags.

Still not sure about the @since tags as even in the files which did have them, not all properties/methods were tagged with them, so the inconsistency is quite grating.

jrfnl · 2016-08-09T13:28:44Z

Grrr... removing the @Package tag at the file level actually broke the build as it's part of the doc standard to have one... so... what shall we do now ?

GaryJones · 2016-08-09T14:45:45Z

removing the @Package tag at the file level actually broke the build as it's part of the doc standard to have one... so... what shall we do now ?

As I said:

@package would be something like WPCS\WordPressCodingStandards IMO.

jrfnl · 2016-08-09T18:15:10Z

@GaryJones That's neither here nor there. I didn't (yet) change the package tag, just removed it from the file level doc block as that @package tag is supposed to apply to global variables, functions etc and we don't have those anyway.

westonruter · 2016-08-17T20:06:09Z

@jrfnl so you could re-add the @package tags as WPCS\WordPressCodingStandards to fix the build, or we could update the phpcs.xml to exclude the Squiz.Commenting.FileComment.MissingPackageTag error code.

I strongly advise for the commits be squashed when merging this, they are just there to show the different iterations and to make reviewing of the changes easier.

I would strongly advise against this, as we should make sure your volume of work is reflected in the contributor graphs 😄

We should then merge this large PR and do some testing with it in develop, and then get 0.10.0 out. The changelog is going to be somewhat of a nightmare to put together! 😄

jrfnl · 2016-08-18T03:31:11Z

@westonruter Any opinion on what the @package tag should be ?

I would strongly advise against this, as we should make sure your volume of work is reflected in the contributor graphs

Please do squash the commits. I really think this should be one commit as it addresses one issue and would otherwise make the history a lot less easy to navigate.
The squashed commit will still have my name on it, so I'm fine with that (I don't have the ego to worry about the stats much anyway).

westonruter · 2016-08-18T04:11:41Z

@jrfnl seems that WPCS\WordPressCodingStandards works for now.

@Package

* Adjusted the @Package tags as per discussion in the PR thread * Removed the @link tag to the handbook at the file level doc block * Removed the @author tags

jrfnl · 2016-08-18T05:37:06Z

Updated the @package tags as per the majority vote and brought them back at file level.
Even though justified, I'd rather WPCS lead by example than make exceptions.

We should then merge this large PR and do some testing with it in develop

As the PR does not contain any code/functional changes, I don't think much - if any - testing is necessary.

jrfnl · 2016-08-18T06:15:11Z

FYI - I've made a start at creating a changelog will push it later today so others can contribute to it as well.

westonruter · 2016-08-18T06:28:31Z

@jrfnl you are amazing. 🙌

jrfnl · 2016-08-18T06:46:15Z

@westonruter 😎 Might as well try to get 0.10.0 out of the door now. We're two weeks past the self-set deadline already.

jrfnl · 2016-08-18T09:09:31Z

FYI: All - I've rebased all my other open PRs to fix any merge conflicts with this one.

@Package

* Update the file and class @Package tag and remove the @category tag. Follow the standard as described in https://www.phpdoc.org/docs/latest/references/phpdoc/tags/package.html The @category tag is considered deprecated. A hierarchical @Package tag should be used instead. Ref: https://www.phpdoc.org/docs/latest/references/phpdoc/tags/category.html * Use correct version number for the newly deprecated classes. * Fix some - unintentional - parse errors in the test files. * Attempt to lead by example comment-wise in the unit tests. Capitalization, punctuation and some spelling. * Proper capitalization and punctuation for class end comments. * Remove `//end` comments for methods and conditionals < 35 lines. * Simplify the test file method doc blocks. * Various documentation fixes, largely based on PHPCS Docs output. * Make sure any function which will be passed the $phpcsFile has the right type hint. But only if it doesn't conflict with upstream function signatures. * Verify and update the docblocks for all files and classes. Based on Git file history: * Add @SInCE tags to all classes. * Add @SInCE tags to properties and methods if they weren't included from the start. * Verify/Fix a number of @author tags. * Remove old/copied over/incorrect PHPCS tags. * Where necessary improved/corrected the class description. Also: * Consistent tag order in class doc blocks. * Fix tag alignment. * Remove redundant explanation in unit test class doc blocks. * Sync the description line of the unit test class doc blocks. * Add handbook links to a number of sniffs. * Add reference to upstream sniff a sniff is based upon. Includes information on when the sniff was last synced with the upstream sniff if applicable and available. * Add @license tag and @link tag to the WPCS GH repo, to all file level doc blocks. License tag as per https://www.phpdoc.org/docs/latest/references/phpdoc/tags/license.html * Add the docs ruleset to the coding standard for WPCS itself. * Add @SInCE class changelogs for PR #647. * Fix minor grammar error. * Updated `@since` tags for `2014-12-11` release to `0.3.0`. * Re-instate previously removed @SInCE tags. * Updated based on feedback. * Adjusted the @Package tags as per discussion in the PR thread * Removed the @link tag to the handbook at the file level doc block * Removed the @author tags

@Package

* Update the file and class @Package tag and remove the @category tag. Follow the standard as described in https://www.phpdoc.org/docs/latest/references/phpdoc/tags/package.html The @category tag is considered deprecated. A hierarchical @Package tag should be used instead. Ref: https://www.phpdoc.org/docs/latest/references/phpdoc/tags/category.html * Use correct version number for the newly deprecated classes. * Fix some - unintentional - parse errors in the test files. * Attempt to lead by example comment-wise in the unit tests. Capitalization, punctuation and some spelling. * Proper capitalization and punctuation for class end comments. * Remove `//end` comments for methods and conditionals < 35 lines. * Simplify the test file method doc blocks. * Various documentation fixes, largely based on PHPCS Docs output. * Make sure any function which will be passed the $phpcsFile has the right type hint. But only if it doesn't conflict with upstream function signatures. * Verify and update the docblocks for all files and classes. Based on Git file history: * Add @SInCE tags to all classes. * Add @SInCE tags to properties and methods if they weren't included from the start. * Verify/Fix a number of @author tags. * Remove old/copied over/incorrect PHPCS tags. * Where necessary improved/corrected the class description. Also: * Consistent tag order in class doc blocks. * Fix tag alignment. * Remove redundant explanation in unit test class doc blocks. * Sync the description line of the unit test class doc blocks. * Add handbook links to a number of sniffs. * Add reference to upstream sniff a sniff is based upon. Includes information on when the sniff was last synced with the upstream sniff if applicable and available. * Add @license tag and @link tag to the WPCS GH repo, to all file level doc blocks. License tag as per https://www.phpdoc.org/docs/latest/references/phpdoc/tags/license.html * Add the docs ruleset to the coding standard for WPCS itself. * Add @SInCE class changelogs for PR #647. * Fix minor grammar error. * Updated `@since` tags for `2014-12-11` release to `0.3.0`. * Re-instate previously removed @SInCE tags. * Updated based on feedback. * Adjusted the @Package tags as per discussion in the PR thread * Removed the @link tag to the handbook at the file level doc block * Removed the @author tags

jrfnl added cleanup Status: Review ready labels Aug 1, 2016

jrfnl added this to the 0.10.0 milestone Aug 1, 2016

jrfnl mentioned this pull request Aug 1, 2016

Question, branch to test projects against? #631

Closed

GaryJones reviewed Aug 1, 2016
View reviewed changes

jrfnl mentioned this pull request Aug 2, 2016

Remove version 2014-12-11 from Packagist? #654

Closed

jrfnl added 16 commits August 4, 2016 20:18

Use correct version number for the newly deprecated classes.

dfc34a0

Fix some - unintentional - parse errors in the test files.

fa4fe74

Attempt to lead by example comment-wise in the unit tests.

e0b8e30

Capitalization, punctuation and some spelling.

Proper capitalization and punctuation for class end comments.

761ba7c

Remove //end comments for methods and conditionals < 35 lines.

44b062b

Simplify the test file method doc blocks.

1f3e22a

Various documentation fixes, largely based on PHPCS Docs output.

8895476

Make sure any function which will be passed the $phpcsFile has the ri…

4b4f60e

…ght type hint. But only if it doesn't conflict with upstream function signatures.

Add handbook links to a number of sniffs.

e783633

Add reference to upstream sniff a sniff is based upon.

5a4e008

Includes information on when the sniff was last synced with the upstream sniff if applicable and available.

Add @license tag and @link tag to the WPCS GH repo, to all file level…

cd8b8aa

… doc blocks. License tag as per https://www.phpdoc.org/docs/latest/references/phpdoc/tags/license.html

Add the docs ruleset to the coding standard for WPCS itself.

7dbf774

Add @SInCE class changelogs for PR #647.

aa2335a

Fix minor grammar error.

e02965d

Updated @since tags for 2014-12-11 release to 0.3.0.

1e7e2a5

jrfnl force-pushed the WPCS/feature/documentation-fixes branch from 26fffa2 to 1e7e2a5 Compare August 4, 2016 18:20

jrfnl added 2 commits August 18, 2016 07:06

Re-instate previously removed @SInCE tags.

fa35ac7

Updated based on feedback.

855736f

* Adjusted the @Package tags as per discussion in the PR thread * Removed the @link tag to the handbook at the file level doc block * Removed the @author tags

jrfnl force-pushed the WPCS/feature/documentation-fixes branch from b6b6d24 to 855736f Compare August 18, 2016 05:34

westonruter merged commit f60bfe5 into develop Aug 18, 2016

jrfnl deleted the WPCS/feature/documentation-fixes branch August 18, 2016 06:14

jrfnl mentioned this pull request Oct 1, 2016

Add sniff to check that <title> tags are not used. WPTT/WPThemeReview#41

Merged

GaryJones removed the Status: Review ready label Aug 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Streamline WPCS in file documentation. #651

Streamline WPCS in file documentation. #651

jrfnl commented Aug 1, 2016 •

edited

Loading

GaryJones Aug 1, 2016

jrfnl Aug 1, 2016

JDGrimes commented Aug 3, 2016 •

edited

Loading

JDGrimes commented Aug 3, 2016

JDGrimes commented Aug 3, 2016

GaryJones commented Aug 3, 2016

JDGrimes commented Aug 3, 2016

JDGrimes commented Aug 3, 2016

GaryJones commented Aug 4, 2016

jrfnl commented Aug 4, 2016

jrfnl commented Aug 4, 2016

GaryJones commented Aug 4, 2016

jrfnl commented Aug 4, 2016

JDGrimes commented Aug 4, 2016

JDGrimes commented Aug 4, 2016

jrfnl commented Aug 4, 2016

jrfnl commented Aug 4, 2016

jrfnl commented Aug 9, 2016

jrfnl commented Aug 9, 2016

GaryJones commented Aug 9, 2016

jrfnl commented Aug 9, 2016

westonruter commented Aug 17, 2016

jrfnl commented Aug 18, 2016 •

edited

Loading

westonruter commented Aug 18, 2016

jrfnl commented Aug 18, 2016

jrfnl commented Aug 18, 2016

westonruter commented Aug 18, 2016

jrfnl commented Aug 18, 2016

jrfnl commented Aug 18, 2016

Streamline WPCS in file documentation. #651

Streamline WPCS in file documentation. #651

Conversation

jrfnl commented Aug 1, 2016 • edited Loading

Overview of changes:

GaryJones Aug 1, 2016

Choose a reason for hiding this comment

jrfnl Aug 1, 2016

Choose a reason for hiding this comment

JDGrimes commented Aug 3, 2016 • edited Loading

JDGrimes commented Aug 3, 2016

JDGrimes commented Aug 3, 2016

GaryJones commented Aug 3, 2016

JDGrimes commented Aug 3, 2016

JDGrimes commented Aug 3, 2016

GaryJones commented Aug 4, 2016

jrfnl commented Aug 4, 2016

jrfnl commented Aug 4, 2016

GaryJones commented Aug 4, 2016

jrfnl commented Aug 4, 2016

JDGrimes commented Aug 4, 2016

JDGrimes commented Aug 4, 2016

jrfnl commented Aug 4, 2016

jrfnl commented Aug 4, 2016

jrfnl commented Aug 9, 2016

jrfnl commented Aug 9, 2016

GaryJones commented Aug 9, 2016

jrfnl commented Aug 9, 2016

westonruter commented Aug 17, 2016

jrfnl commented Aug 18, 2016 • edited Loading

westonruter commented Aug 18, 2016

jrfnl commented Aug 18, 2016

jrfnl commented Aug 18, 2016

westonruter commented Aug 18, 2016

jrfnl commented Aug 18, 2016

jrfnl commented Aug 18, 2016

jrfnl commented Aug 1, 2016 •

edited

Loading

JDGrimes commented Aug 3, 2016 •

edited

Loading

jrfnl commented Aug 18, 2016 •

edited

Loading