Add GitHub push action enforcing Prettier style

[eemeli]

The styles we use to write markdown vary, and the result is occasionally a bit messy.

Prettier defines some pretty decent defaults, and using them allows us to care less about the style we use. This PR adds a GitHub Action that runs Prettier after every commit that touches a .md file and pushes any changes to the current branch with a style: Apply Prettier commit message.

The first commit here fixes the one place where the styling appeared to cause an issue in spec/syntax.md; the second commit has been automatically generated by the action.

[CLAassistant]

All committers have signed the CLA.

[eemeli]

It looks like the action's email address will need to match one that's acceptable to the CLA bot.

@aphillips @macchiati Is there any such bot email address yet that could be used?

[srl295]

I'll add it as an exception.
Same as dependabot.

Ok to fix tomorrow?

[eemeli]

Tomorrow's completely fine; this is fixing a long-running annoyance.

Right now the email address is empty in the generated commits, so please tell me which address to use once you've got it set up? For example, [email protected] could work.

[srl295]

I don't know what a good email would be. But i wonder if actions would be a better username since that's already reserved. Someone could sign up and spoof this one!

Anyway i added it to the allow list so you should be able to pass this

[srl295]

LGTM can update the id later

[eemeli]

I set the action to use github-actions <[email protected]>. Not sure if the CLA checked has re-run itself though?

[srl295]

github-actions seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

[srl295]

I set the action to use github-actions <[email protected]>. Not sure if the CLA checked has re-run itself though?

seemed to make it not work… i just added github-actions as the exclusion (by the github id not the email)

[srl295]

🎉

[eemeli]

Phew, I think it's now sorted? Thank you @srl295 for your help here!

[srl295]

seems ok

[srl295]

💯 for persistence

[stasm]

This PR adds a GitHub Action that runs Prettier after every commit that touches a .md file and pushes any changes to the current branch with a style: Apply Prettier commit message.

Please don't auto-commit to main like this. It will generate needless diffs, making browsing the history and blaming files more difficult. Instead, let's set up a CI check on all PRs and require it to pass before merging. All changes to the spec should go through PRs anyways.

[eemeli]

This would have the same effect on PRs as a CI check, because it's run on all pushes on all forks. The benefit of doing it this way is that the style corrections are automatic rather than blocking.

For example, if a PR review suggestion commit introduces some trailing whitespace to a spec PR, this would immediately fix that, rather than blocking the PR with a failing check which would need manual action.

The only way this would add a spec/ changing commit directly on main is if someone else first edits it directly, without going through the PR process.

[srl295]

This PR adds a GitHub Action that runs Prettier after every commit that touches a .md file and pushes any changes to the current branch with a style: Apply Prettier commit message.

Please don't auto-commit to main like this. It will generate needless diffs, making browsing the history and blaming files more difficult. Instead, let's set up a CI check on all PRs and require it to pass before merging. All changes to the spec should go through PRs anyways.

that's what CLDR and Unicodetools do. if you forget, your PR just doesn't pass until it's fixed.

Auto committing to a PR seems reasonable.

[stasm]

This would have the same effect on PRs as a CI check, because it's run on all pushes on all forks. The benefit of doing it this way is that the style corrections are automatic rather than blocking.

Oh, do you mean that there will be automatic commits in the PR's branch, but since we merge by squashing, they will never make it to main?

[srl295]

@eemeli i would recommend having the action only apply to PRs and not to main.

[macchiati]

We chose in CLDR and ICU to have the equivalent to Prittier be run as a test, and fail if the format would change. That way what the author has in the PR is exactly what is merged in. It does place the onus on the author to run the prettier before committing to a PR, but has a cleaner record in github.

…

On Wed, Aug 16, 2023 at 3:02 PM Stanisław Małolepszy < ***@***.***> wrote: This would have the same effect on PRs as a CI check, because it's run on all pushes on all forks. The benefit of doing it this way is that the style corrections are automatic rather than blocking. Oh, do you mean that there *will* be automatic commits in the PR's branch, but since we merge by squashing, they will never make it to main? — Reply to this email directly, view it on GitHub <#451 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACJLEMFYMDFTTWNMCBKIGOTXVU7ORANCNFSM6AAAAAA3QMSJZM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[eemeli]

@stasm:
Oh, do you mean that there will be automatic commits in the PR's branch, but since we merge by squashing, they will never make it to main?

Yes. Going forward, to submit a PR you'll need a branch here or a fork of this repo that'll include the .github/workflows/prettier.yaml that's added by this PR, and it'll trigger the action which will (if necessary) add a commit to the PR branch with the style fixes.

@srl295:
i would recommend having the action only apply to PRs and not to main.

The problem is that if/as we do occasionally commit directly to main e.g. most recently with 828d4cb adding meeting notes, if those commits do introduce something that doesn't match the Prettier styles, they would get picked up by all subsequent PR branches until one of them is merged -- it's much trickier to configure Prettier to only apply itself to the files actually changed on a branch, rather than all files.

Allowing commits also on main only really affects meeting notes and similar administrativia, not the spec. If we do want to prevent the action from running on main, then really we should protect the branch and require all its changes to land via PRs. This would probably inconvenience @aphillips more than anyone else; do you have an opinion on such a change?

[stasm]

As agreed in today's meeting, let's try this as-is and see how many auto-commits actually make it to main.