This repository has been archived by the owner on Feb 21, 2025. It is now read-only.
Add automatic Prettier GitHub action #4259
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
felixfbecker
force-pushed
the
prettier
branch
2 times, most recently
from
ae02ae6 to
677ecd3
Compare
|
Notifying subscribers in CODENOTIFY files for diff 77d1022...c9cfc9a.
|
|
As someone who often forgets to run prettier (because of infrequency of updates and/or having to push something between meetings) this would be awesome! |
courier-new
approved these changes
Aug 20, 2021
There was a problem hiding this comment.
What a great idea!
For (3), Git supports the option --ignore-revs-file (also configurable globally with git config) that has been adopted in open source projects already (e.g. https://github.com/v8/v8/blob/master/.git-blame-ignore-revs). After merging this PR, I will add such a file with the commit hash of the format commit, which would still allow us to blame, ignoring the first large format commit. I will also say though that I think we should do this regardless because with frequent changes to the handbook eventually the commit will be long forgotten about anyway and we do this all the time in our main repo too, even without a blame-ignore-revs file, e.g. every time we upgrade ESLint. The benefits of editing experience outweigh the cost.
😱 TIL. We should absolutely start doing this when we upgrade ESLint or something similar in the main repo, too.
valerybugakov
approved these changes
Aug 23, 2021
Using the GITHUB_TOKEN causes no workflows to be run on the pushed commit
|
This broke nested list formatting in our handbook. What is the right way to fix this?
|
|
@nicksnyder The right "fix" would be to migrate our Markdown renderer from Introducing Prettier of course made this issue a lot more apparent and I'm sorry for that – I didn't think of this bug when adding it. I still think it's the right thing to have Prettier though considering the benefits and that this was a frequent (although slightly less frequent) problem before. I investigated several workarounds (including changing indentation in the Prettier config, which didn't work as expected) and I think the best option is to just use Btw, this even a bug in our Markdown rendering in our product too, because we use |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overall, the goal of this PR is to make the content in this repo a lot easier to edit and read without getting into anyone's way.
Currently, editing CSS or JS (and even Markdown in some cases) for the handbook or the website is a really frustrating experience. Indendation is inconsistent between 2 and 4, sometimes tabs and spaces are mixed within the same file. Selectors are sometimes formatted in a hard to read way. TSX needs to be indented manually.
There is trailing whitespace everywhere, especially in Markdown files. It is extremely difficult to not end up with huge diffs (and consequential merge conflicts) on every PR because editors auto-trimmed whitespace without having to jump through many hoops to reconfigure editors just when editing the handbook.
Markdown is formatted in inconsistent ways when Markdown supports multiple ways to do the same thing (lists, bolding, italics, ...), making it harder to read and understand the plain text source, sometimes even leading to errors.
Markdown tables are effectively unreadable (and uneditable) in their source because the columns are completely misaligned (and who has time to format them manually?).
All of this could be easily solved by Prettier with autoformatting. What speaks against that is:
maindirectly anymore, meaning every change needs to go through a PRgit blameharder to traceI believe all of these points are solvable, and that this PR addresses each.
It adds a Prettier GitHub action that will automatically reformat files on PRs and push a commit to the branch, meaning people don't need to worry about setting up a local editor and can even continue to use the GitHub UI for edits. On merge the commit is squashed so there will be no trace of it on
main.We've already enforced PRs and required status checks for a while now after many breakages had happened from pushing to
mainthat would cause subsequent changes and the deploy to be blocked. I consider this a positive change for many reasons, as overall it also makes sure as our organization scales edits to the handbook are done in PRs that can be reviewed and commented on by anyone at the company.For (3), Git supports the option
--ignore-revs-file(also configurable globally withgit config) that has been adopted in open source projects already (e.g. https://github.com/v8/v8/blob/master/.git-blame-ignore-revs). After merging this PR, I will add such a file with the commit hash of the format commit, which would still allow us to blame, ignoring the first large format commit. I will also say though that I think we should do this regardless because with frequent changes to the handbook eventually the commit will be long forgotten about anyway and we do this all the time in our main repo too, even without ablame-ignore-revsfile, e.g. every time we upgrade ESLint. The benefits of editing experience outweigh the cost.