Skip to content
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

Closed
wants to merge 4 commits into from
Closed

Conversation

nschonni
Copy link
Contributor

@nschonni nschonni commented Nov 8, 2020

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:

  • Match indenting in EditorConfig of 2 spaces
  • Add missing EOF
  • Collapse empty arrays and objects

Check off the following:

@nschonni nschonni requested a review from a team as a code owner November 8, 2020 07:36
Copy link
Contributor

@heiskr heiskr left a 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!

@janiceilene janiceilene added the engineering Will involve Docs Engineering label Nov 9, 2020
@chiedo
Copy link
Contributor

chiedo commented Nov 10, 2020

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.

@chiedo chiedo added the blocked This issue or PR is currently blocked label Nov 10, 2020
@github-actions
Copy link
Contributor

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.

@github-actions github-actions bot added the stale There is no recent activity on this issue or pull request label Nov 17, 2020
@nschonni
Copy link
Contributor Author

@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?

@nschonni nschonni changed the title chore: Format json files with jsonfmt chore: Format json files with Prettier Nov 17, 2020
@chiedo
Copy link
Contributor

chiedo commented Nov 18, 2020

just ignore a particular pattern of generated files

My opinion is this!

@github-actions github-actions bot removed the stale There is no recent activity on this issue or pull request label Nov 18, 2020
@nschonni
Copy link
Contributor Author

@chiedo if you provide a pattern I can add it to the .prettierignore. Flip side is that now that this is automated and linted, it's easier to re-apply to the generated files as they land, even if they tool produces something slightly different natively

@chiedo
Copy link
Contributor

chiedo commented Nov 18, 2020

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.

@nschonni
Copy link
Contributor Author

@rachmari I took your approach for the Yaml files in update-files.js and added the json extension 😄

@chiedo
Copy link
Contributor

chiedo commented Dec 1, 2020

@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 chiedo closed this Dec 1, 2020
@nschonni
Copy link
Contributor Author

nschonni commented Dec 1, 2020

@chiedo this hooks into the script like the Yaml files for the generated content

Octomerger pushed a commit that referenced this pull request Dec 7, 2021
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked This issue or PR is currently blocked engineering Will involve Docs Engineering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants