-
Notifications
You must be signed in to change notification settings - Fork 335
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 PR diff comments for npm package changes #4511
Conversation
📋 StatsFile sizes
Modules
View stats and visualisations on the review app Action run for 8881365 |
9661583
to
3a76c66
Compare
3a76c66
to
abdfc21
Compare
JavaScript changes to npm packageNo changes found. Action run for 8881365 |
Stylesheets changes to npm packageNo changes found. Action run for 8881365 |
Other changes to npm packageNo changes found. Action run for 8881365 |
0f64dbf
to
1c37939
Compare
ec8a02d
to
122efff
Compare
4ea1351
to
d17acc2
Compare
fd8e14e
to
a6fc4c4
Compare
d17acc2
to
7591008
Compare
f4af7a6
to
57453be
Compare
Picks up 2-space indent via `--editorconfig` and clearly shows document `--type` for JS or CSS
45fa227
to
e53d6e9
Compare
e53d6e9
to
f514d76
Compare
f514d76
to
1bca0a6
Compare
1bca0a6
to
20ea8cc
Compare
20ea8cc
to
94b5086
Compare
94b5086
to
d93603d
Compare
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 think this is great - imho, more information is always good.
Coupla thoughts:
- Are other devs OK with the increased noise (I say, let's merge and see...)
- Are the comments always going to generate in order? For example, it's weird to see "Other changes" first on 🚀 Pre-release v5.0.0-internal.0 #4373
- I wonder if it's worth rethinking the grouping down the line - feels slightly strange to see CSS and SCSS changes split up on Fix header product name wrapping onto new line #4498 , especially since they directly relate to each other. That could be a whole spiral of work though.
- Another potential future idea: Can we merge in the size stats with these comments? Only highlight a size change if there's some diff in the relevant files? Should we have a comment for each of the files currently in the stats comment, if they have a difference?
# Using `origin/$GITHUB_BASE_REF` to avoid actually checking out the branch | ||
# as all we need is to let Git diff the two references | ||
bin/dist-diff.sh origin/$GITHUB_BASE_REF $GITHUB_WORKSPACE | ||
git config user.name github-actions |
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.
Are these needed because of the move to a workflow_call
?
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.
Ah this is a long story
We know we want to use git diff
for js-beautify
formatting but the npm package isn't committed
Running git diff
on uncommitted files
Since we need to build both the "before" (base) and "after" (head) npm packages:
Idea 1: Build into two directories and run git diff --no-index
Idea 2: Build + commit twice, then run git diff HEAD^ HEAD
on the last 2 commits
Sadly we can't use ":(exclude)**/*.min.*"
pathspec with git diff --no-index
But to commit each build we need user.name
and user.email
even though they never go anywhere
Thanks @domoscargin Appreciate the code review
I'm happy with the noise to avoid accidental npm package changes creeping in such as:
We can remove the "No changes found" default in future perhaps?
☝️ Ah I hadn't noticed this but it'll be a side effect of racing them to the finish in: govuk-frontend/.github/workflows/scripts/comments.mjs Lines 15 to 16 in 6c712d7
I won't merge this PR just yet until we get some more feedback Like all the future ideas though |
d93603d
to
8881365
Compare
This PR adds "Diff changes to package" to the Tests GitHub Actions workflow to close #4258
This new workflow adds
git diff
comments for packages/govuk-frontend/dist in every PRDiff comment examples
Have a look at 1x small and 1x big example:
*.d.mts
#4094 (comment)PR diff comments follow the same approach as #4039