-
Notifications
You must be signed in to change notification settings - Fork 60.5k
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
chore: Format json files with Prettier #1177
Conversation
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.
Makes sense to me. 🙏🏼 Thanks!
I connected with @sarahs and most of these files are generated automatically by scripts... it wouldn't hurt anything to merge this, but the files will just get overwritten with the old whitespace at some point. So I'm not clear what the benefit to merging this would be. |
This PR is stale because it has been open 7 days with no activity and will be automatically closed in 3 days. To keep this PR open, update the PR by adding a comment or pushing a commit. |
@chiedo now that the Prettier for Yaml PR landed, would it make sense to add that for the JSON files too or just ignore a particular pattern of generated files? |
My opinion is this! |
@chiedo if you provide a pattern I can add it to the |
Thanks @nschonni! I'll follow back up in a few weeks. We're going to start reviewing all open source PRs on Mondays (starting after Thanksgiving) as a team. We've had a few workflow changes break internal processes over the past week so we're going to start looping everyone in to get sign off. |
Removed comments from .devcontainer.json
@rachmari I took your approach for the Yaml files in |
@nschonni sorry on the delay here! Most of these JSON files are generated via automation so we don’t see a pressing reason to merge this PR as they’ll get overwritten. So we’re going to close this PR for the time being. Moving forward can you start by opening an issue rather than a PR so we can make sure we can commit to the issue before you start working on it? |
@chiedo this hooks into the script like the Yaml files for the generated content |
* Experiment with making the tarball smaller Part of #1248 * try this * stop debugging * delete translations too * delete heavy search indexes too * push and popd * try this hack * delete but leave directory * debug more * faster delete of translations * less loud * async await * async await * no tree * simplify * experimenting more * unfinished * only the large files * change order * brotli with level 6 * cope better with decorated rest json files * tidying * keep images * cleaning * cleaning up * refactored function * try this * better comment * remove console logging * more important changes * make lib/rest/index.js callable Part of #1177 * memoize * remove console logging
Why:
EditorConfig was added in a previous PR. This does a one time cleanup so the next changes to the files are a simpiler diff that won't have to include any of the whitespace cleanups
What's being changed:
Probably easiest to view in the "whitespace ignored view" https://github.com/github/docs/pull/1177/files?diff=unified&w=1
Ran https://github.com/caarlos0/jsonfmt as a quick why to consistently apply the whitespace as a on-off instead of trying to introduce something with a lot of opinons like Prettier:
Check off the following: