Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a package to hold our coding standard #17405

Merged
merged 1 commit into from
Oct 19, 2020
Merged

Conversation

anomiex
Copy link
Contributor

@anomiex anomiex commented Oct 7, 2020

Changes proposed in this Pull Request:

This moves the code for custom Jetpack phpcs sniffs into a new package in the monorepo, which will make it easier in the future to import additional sniffs and add tests. It'll also make it easy to reuse the coding standard in any other code repo we might wind up with.

This also adds some tests, and has the WordPress sniffs be pulled in via the Jetpack standard instead of being pulled in on their own.

Jetpack product discussion

None directly, but see p9dueE-1Y9-p2 for the proposal this cleanup is motivated by.

Does this pull request change what data or activity we track or use?

No.

Testing instructions:

  • Verify that this doesn't change any sniffs reported by composer php:lint or the like. You might do that by
    • Check out a clean copy of master. Run composer install && composer php:lint -- --report-file=/tmp/report.old
    • Pull in this PR. Run composer install && composer php:lint -- --report-file=/tmp/report.new
    • Compare /tmp/report.old and /tmp/report.new to see if any unexpected changes were made.

Proposed changelog entry for your changes:

  • This is developer-only, so probably no need for a changelog entry.

@anomiex anomiex added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Type] Janitorial [Pri] Normal labels Oct 7, 2020
@anomiex anomiex requested review from jeherve and leogermani October 7, 2020 21:07
@anomiex anomiex self-assigned this Oct 7, 2020
@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 Oct 7, 2020
@jetpackbot
Copy link

jetpackbot commented Oct 7, 2020

Scheduled Jetpack release: November 10, 2020.
Scheduled code freeze: November 3, 2020

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-17405

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.

Generated by 🚫 dangerJS against cadfcaa

@jeherve jeherve added this to the 9.1 milestone Oct 8, 2020
@kraftbj
Copy link
Contributor

kraftbj commented Oct 16, 2020

This can replace #15693 in principle. There is a rule in there that we'd need to bring over, but I like this approach to creating the package more than what I had.

@anomiex
Copy link
Contributor Author

anomiex commented Oct 19, 2020

This can replace #15693 in principle. There is a rule in there that we'd need to bring over, but I like this approach to creating the package more than what I had.

Ooh, someone tried this before. Nice! Looking for differences... It looks like we did mostly the same things.

This will make it a bit cleaner in the future to manage tests and
dependencies for the coding standard.

And, if some other repo wants to use our coding standards, they can more
easily do so.

The new coding standard package imports the PHPCompatibility and
WordPress standards, so those are removed from `.phpcs.xml.dist`.
The phpcs configuration specific to Jetpack remains in
`.phpcs.xml.dist`.
@anomiex anomiex force-pushed the add/codesniffer-package branch from 7eec79d to cadfcaa Compare October 19, 2020 17:09
@kraftbj
Copy link
Contributor

kraftbj commented Oct 19, 2020

Yup, I'm fine waiting to add rules until after this lands.

Copy link
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

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

Let's give this a spin.

@anomiex anomiex merged commit a8babfb into master Oct 19, 2020
@anomiex anomiex deleted the add/codesniffer-package branch October 19, 2020 20:39
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 19, 2020
@jeherve jeherve added [Package] Codesniffer and removed [Status] Needs Package Release This PR made changes to a package. Let's update that package now. labels Oct 20, 2020
@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 Oct 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Codesniffer [Pri] Normal [Status] Needs Package Release This PR made changes to a package. Let's update that package now. [Type] Janitorial
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants