Skip to content

Conversation

@kraftbj
Copy link
Contributor

@kraftbj kraftbj commented May 6, 2020

When writing up some documentation for a new filter in 8.5, I found a hook docblock that had the incorrect version number (8.5 vs 8.5.0) and wanted to prevent that from happening in the future.

This is a first pass at a Coding Standards package for Jetpack.

It still needs unit tests and, ideally, PHPCS-compliant docs.

Changes proposed in this Pull Request:

  • Modifies the Jetpack .phpcs.xml.dist for jetpack repo-specific needs.
  • Excludes the package from some WPCS rules to match PHPCS style.
  • Creates a "Jetpack" ruleset that can be included by other projects via Composer.
  • The Jetpack ruleset is the existing WP coding standards, with the following additions:
    ** Verifies that hook execution function calls (apply_filters, do_action, etc) are preceded by a docblock comment.
    ** Verifies that the docblock comment has a @since tag and a @module tag.

I believe there are plenty of places in Jetpack where we don't assign a module to a filter, so that is something we can remove.

The @since enforcement will force a X.Y.Z version while still allow an annotation following it (e.g. both @since X.Y.Z and @since X.Y.Z Changes default are allowed).

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • Adds a new package. Not discussed on a p2.

Testing instructions:

  • composer install
  • Add apply_filters or one of the other hook execution functions. Ensure the tests flag lack of docblock.
  • Add a docblock with various levels of completeness. Confirm it flags it.
  • Add a /** This filter is documented in XYZ */ style line. Confirm it passes.

Proposed changelog entry for your changes:

  • Package: Adds a Jetpack Coding Standards package.

@kraftbj kraftbj added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] In Progress [Focus] Jetpack DNA labels May 6, 2020
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello kraftbj! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D42980-code before merging this PR. Thank you!
This revision will be updated with each commit to this PR

@github-actions github-actions bot added the [Status] Needs Package Release This PR made changes to a package. Let's update that package now. label May 6, 2020
@jetpackbot
Copy link
Collaborator

jetpackbot commented May 6, 2020

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: June 2, 2020.
Scheduled code freeze: May 26, 2020

Generated by 🚫 dangerJS against 56eded0

/*
* Check to determine if there is a comment immediately preceding the function call.
*/
if ( ( $this->tokens[ $previous_comment ]['line'] + 1 ) !== $this->tokens[ $stack_ptr ]['line'] ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should double-check whether DocBlock association needs to be on the immediate line before, or just the thing before it when whitespace is excluded. i.e.:

/**
 * ...
 */
apply_filters( ... );
/**
 * ...
 */

apply_filters( ... );

Both may be correctly associated. It should have parity with the (draft) PSR-5 (my emphasis):

It is RECOMMENDED to precede a "Structural Element" with a DocBlock where it is defined and not with each usage. It is common practice to have the DocBlock precede a Structural Element but it MAY also be separated by an undetermined number of empty lines.

<file>.</file>

<!-- Show sniff codes in all reports -->
<arg value="s"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for not including p to show progress?

Copy link
Contributor

@GaryJones GaryJones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting a change as the XML is invalid (first inline comment).

@kraftbj kraftbj marked this pull request as draft May 27, 2020 22:13
@kraftbj kraftbj self-assigned this May 27, 2020
@stale
Copy link

stale bot commented Aug 29, 2020

This PR has been marked as stale. This happened because:

  • It has been inactive in the past 3 months.
  • It hasn’t been labeled `[Pri] Blocker`, `[Pri] High`.

No further action is needed. But it's worth checking if this PR has clear testing instructions, is it up to date with master, and it is still valid. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Focus] Jetpack DNA [Status] Needs Package Release This PR made changes to a package. Let's update that package now. [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants