Skip to content

Conversation

@noahtallen
Copy link
Contributor

@noahtallen noahtallen commented Jul 12, 2023

Related to: #79266

Proposed Changes

Follow-up from #79097 (comment), to add extra checks for ETK's text domains.

The new script only succeeds if all of the following are true:

  1. synced blocks exist
  2. dist build files exist
  3. a call to eslint contains "No lint failures found for rule(s): @wordpress/i18n-text-domain"
  4. phpcs linted the same number of .php files that exist in ETK
  5. phpcs has a zero exit code

(This phpcs call uses a custom config which only includes the i18n rule.)

It fails otherwise.

Proof

Proof that it catches issues with JS textdomain in CI: https://teamcity.a8c.com/buildConfiguration/calypso_WPComPlugins_EditorToolKit/10371617?hideProblemsFromDependencies=false&hideTestsFromDependencies=false&expandBuildChangesSection=true&showLog=10371617_801_752&logView=flowAware

Proof that it catches issues with PHP textdomain in CI: https://teamcity.a8c.com/buildConfiguration/calypso_WPComPlugins_EditorToolKit/10371663?buildTab=overview&hideProblemsFromDependencies=false&hideTestsFromDependencies=false&expandBuildChangesSection=true&showLog=10371663_1390_962&logView=flowAware

Testing Instructions

  1. run yarn distclean then yarn install
  2. cd to apps/editing-toolkit
  3. run ./bin/verify-textdomain.sh -- it should fail since the blocks are not synced and JS bundles don't exist.
  4. run yarn build
  5. run the script again and it should succeed
  6. Create a textdomain issue in a JS file: apps/editing-toolkit/editing-toolkit-plugin/newspack-blocks/synced-newspack-blocks/blocks/homepage-articles/edit.js
  7. run the script and it should fail
  8. Create a textdomain issue in one of the synced PHP files. (Or any ETK file). You might need to add your own line including __.
  9. run the script and it should fail

@noahtallen noahtallen requested a review from a team July 12, 2023 02:52
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 12, 2023
@github-actions
Copy link

github-actions bot commented Jul 12, 2023

@noahtallen noahtallen self-assigned this Jul 12, 2023
@noahtallen noahtallen requested a review from tyxla July 12, 2023 02:53
@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Thank you for taking care of this one so quickly, @noahtallen! 🙌

It looks great - let's give it a try! 🚀

The best way would be opening a couple of PRs, one that breaks it and one that doesn't, and demonstrating that CI will fail in one of them and succeed in the other.

Copy link
Member

Choose a reason for hiding this comment

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

I greatly appreciate we're using eslint-nibble! It's much better and less fragile if we precisely fix only this rule and no others.

@noahtallen
Copy link
Contributor Author

The best way would be opening a couple of PRs, one that breaks it and one that doesn't, and demonstrating that CI will fail in one of them and succeed in the other.

I did demonstrate this in a few commits above! You can see the failing checks ;)

@noahtallen noahtallen force-pushed the add-more-newspack-blocks-fixes branch from 13013ba to af2b5d7 Compare July 13, 2023 18:42
@matticbot
Copy link
Contributor

This PR modifies the release build for editing-toolkit

To test your changes on WordPress.com, run install-plugin.sh editing-toolkit add-more-newspack-blocks-fixes on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-mMA-p2

@noahtallen noahtallen merged commit 0644f28 into trunk Jul 13, 2023
@noahtallen noahtallen deleted the add-more-newspack-blocks-fixes branch July 13, 2023 19:09
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 13, 2023
@noahtallen
Copy link
Contributor Author

Verifying it again: #79385

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants