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 more newspack blocks fixes #79272

Merged
merged 11 commits into from
Jul 13, 2023
Merged

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.

fi

set +e
eslint_result=$(npx eslint-nibble --no-interactive --rule '@wordpress/i18n-text-domain' .)
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.

3 participants