-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Run PHPCS in CI for FSE plugin #43458
Conversation
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. |
Caution: This PR affects files in the FSE Plugin on WordPress.com D45154-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing in the FSE Plugin for more info: PCYsg-ly5-p2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
paths: | ||
- 'apps/full-site-editing/full-site-editing-plugin/**' | ||
- 'apps/full-site-editing/**' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we should build and sync diff creation for things outside of actual plugin folder. 🤔 It probably won't cause issues since we only merge version bump PRs anyway. Another option would be to isolate the changes here in a separate workflow file with these broader matchers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That might be possible. IMO, we should be running:
yarn build
phpunit
phpcs
to any change in apps/full-site-editing (e.g. to pick up on config changes). And on changes to yarn-lock.
IMO the sync diff is the exception 🤔 I wonder if we can restrict a job based on the path itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we should build and sync diff creation for things outside of actual plugin folder. It probably won't cause issues since we only merge version bump PRs anyway.
Yeah, and there's currently not a lot of files in apps/full-site-editing/
(outside of apps/full-site-editing/full-site-editing-plugin
), so the overhead shouldn't be too bad.
Another option would be to isolate the changes here in a separate workflow file with these broader matchers.
That might make sense in the long run.
Personally, I wouldn't block merging this PR on that -- I think it's important to get PHPCS into CI, and we can tweak and iterate how we do it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the sync diff is the exception 🤔 I wonder if we can restrict a job based on the path itself.
Afaik that hasn't been introduced in GH actions proper yet, although I've seen some 3rd party actions for it (I would rather avoid that dependency).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't consider this a blocker either, just a possible future improvement.
yarn.lock
Outdated
@@ -1,4 +1,4 @@ | |||
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. | |||
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. ars |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍑
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This is going to be very valuable for FSE.
There's been a few good suggestions on this PR, but I think it's important to get PHPCS into CI soon. We can then tweak it in later iterations.
paths: | ||
- 'apps/full-site-editing/full-site-editing-plugin/**' | ||
- 'apps/full-site-editing/**' | ||
- 'yarn.lock' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out that this match is too broad and it will trigger on PRs that are not related to FSE plugin. For example see D45192-code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll file a PR to remove it 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addresses #43458 (comment): In #43458, we added a job to PHPCS lint any changed files in the FSE plugin directory. Additionally, we started triggering that action (that's also in charge of creating the FSE diff for WP.com) upon changes to `yarn.lock`. Turns out that this is too broad a criterion, since `yarn.lock` will change everytime e.g. a Renovate PR is created -- thus creating those FSE diffs too often. While we can consider triggering diff generation upon npm dependency changes, that is an orthogonal change to adding PHPCS linting, so IMO it's fine to remove it -- it's not a regression over the GH Action's state prior to #43458.
Changes proposed in this Pull Request
Testing instructions
I verified that this does the following: