Skip to content

Format the spec with prettier#727

Merged
leebyron merged 2 commits intographql:mainfrom
benjie:prettier
Jan 6, 2022
Merged

Format the spec with prettier#727
leebyron merged 2 commits intographql:mainfrom
benjie:prettier

Conversation

@benjie
Copy link
Copy Markdown
Member

@benjie benjie commented Jun 2, 2020 •
edited
Loading

This PR has been work in progress for a while; and a lot has changed. Please see the edit history for previous versions of this comment.

Thanks to advances to spec-md, we can now format the GraphQL spec with prettier! This means consistent formatting throughout the document, and no need for PR comments about there being too many or two few linebreaks 😉

Prettier has been desired in the GraphQL spec for almost 4 years - I'm excited to have worked with @leebyron over the past year to have finally made this a reality! 🎉

This PR is formed of two commits:

  1. The first commit adds the tooling around prettier, including the yarn format (or npm run format) command which formats the repository.
  2. The final commit simply runs yarn format and commits the results.

To view the HTML diff:

  1. checkout main
  2. run yarn build
  3. run mv public/draft/index.html before.html (using mv will also ensure that the old output cannot be reused)
  4. checkout the final commit
  5. run yarn build
  6. run mv public/draft/index.html after.html
  7. At this point, before.html and after.html differ due to different data-source URLs; let's remove these:
  8. run cat before.html | sed 's/ data-source="[^"]*"//g' > before-nosource.html
  9. run cat after.html | sed 's/ data-source="[^"]*"//g' > after-nosource.html

You can now see the versions with data-source removed are identical:

$ shasum before.html after.html before-nosource.html after-nosource.html
2380c111a29891fa4e8547ef5cee32d429299982  before.html
280252b0fe1aac6065cb280470efb1a096447fa4  after.html
09a7ae84464cc49f6affab9bd544ad4d687837a1  before-nosource.html
09a7ae84464cc49f6affab9bd544ad4d687837a1  after-nosource.html

This was referenced Jun 2, 2020
Merged
Merged
@benjie

This comment has been minimized.

Sign in to view
Base automatically changed from master to main February 3, 2021 04:50
@benjie benjie mentioned this pull request Apr 2, 2021
Merged
benjie
benjie commented Apr 5, 2021
View reviewed changes
Copy link
Copy Markdown
Member Author

@benjie benjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now reviewed all the non-spec markdown changes (spec markdown changes were validated by asserting that there is no diff to the generated HTML) and am happy that the changes are fine 👍

Comment thread rfcs/InputUnion.md Outdated
| Typescript | [Discriminated Union](http://www.typescriptlang.org/docs/handbook/advanced-types.html#discriminated-unions) | ✅ | ✅ |
| Rust | [Enum](https://doc.rust-lang.org/rust-by-example/custom_types/enum.html) | ✅ | ✅ |
| Swift | [Enumeration](https://docs.swift.org/swift-book/LanguageGuide/Enumerations.html) | ✅ | ✅ |
| Haskell | [Algebraic data types](http://learnyouahaskell.com/making-our-own-types-and-typeclasses) | ✅ | ✅ |
Copy link
Copy Markdown
Member Author

@benjie benjie Apr 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is such an improvement!

@benjie benjie marked this pull request as ready for review April 5, 2021 10:30
@benjie
Copy link
Copy Markdown
Member Author

benjie commented Apr 5, 2021 •
edited
Loading

I recommend that #832 is merged separately before this PR to make reviewing the git history more straightforward.

@leebyron leebyron added this to the May2021 milestone Apr 6, 2021
@leebyron
Copy link
Copy Markdown
Collaborator

leebyron commented Apr 6, 2021

@benjie should we merge this one after going through the rest of the pre-cut editorial process? I'm marking PRs I want to place attention on as clear candidates for the next spec cut and anticipate prettier editing merge conflicts.

Inevitably there will be open PRs that will need to rebase and run prettier, but maybe we should try to limit most of those for after the spec cut?

@leebyron leebyron added the ✏️ Editorial PR is non-normative or does not influence implementation label Apr 6, 2021
@benjie
Copy link
Copy Markdown
Member Author

benjie commented Apr 6, 2021

@leebyron Yes, lets do this as the very last step before the spec cut 👍 Given the proof that there's no diff currently I think we can expect that changes will be minimal (code formatting, for example). I'm happy to rebase/fix/etc this PR when the time comes (or you can; the final commit is auto-generated so just don't pick that one, instead generate it again manually).

@leebyron
Copy link
Copy Markdown
Collaborator

leebyron commented Apr 13, 2021

We might want to keep an eye on prettier/prettier#10655 being released

@leebyron leebyron added 🐝 Process Related to Governance, Tools, or other meta work ✏️ Editorial PR is non-normative or does not influence implementation and removed ✏️ Editorial PR is non-normative or does not influence implementation labels Apr 22, 2021
@leebyron leebyron force-pushed the main branch 4 times, most recently from e5d241d to 6c81ed8 Compare April 23, 2021 19:15
@leebyron leebyron removed this from the May2021 milestone May 20, 2021
@benjie
Copy link
Copy Markdown
Member Author

benjie commented May 26, 2021

I've rebased this, upgraded to the latest prettier, and reapplied the changes. Asserting there's no diff is a little more complex now because of the new data-source that spec-md is adding; however:

git checkout main
npm install
npm run build && cp out/index.html before-prettier.index.html
git checkout prettier
npm install
npm run build && cp out/index.html after-prettier.index.html
cat before-prettier.index.html | sed 's/ data-source="[^"]*"//g' > before-prettier-nosource.index.html 
cat after-prettier.index.html | sed 's/ data-source="[^"]*"//g' > after-prettier-nosource.index.html
shasum before-prettier.index.html after-prettier.index.html before-prettier-nosource.index.html after-prettier-nosource.index.html

shows that, ignoring data-source the generated HTML is identical:

c0612337b3750da593fcd050770c1d492d964434  before-prettier.index.html
7d3b2d968f2e2046323a6fbeec39e223f29fbd96  after-prettier.index.html
466a3c4940d848fa2ef5cf432d602f6e7acebf8c  before-prettier-nosource.index.html
466a3c4940d848fa2ef5cf432d602f6e7acebf8c  after-prettier-nosource.index.html

I believe this is safe to merge now 👍

@benjie benjie requested a review from leebyron May 26, 2021 16:20
@netlify
Copy link
Copy Markdown

netlify Bot commented Jan 6, 2022 •
edited
Loading

✔️ Deploy Preview for graphql-spec-draft ready!

🔨 Explore the source changes: b444b37

🔍 Inspect the deploy log: https://app.netlify.com/sites/graphql-spec-draft/deploys/61d6e7016a85e900079ffd6d

😎 Browse the preview: https://deploy-preview-727--graphql-spec-draft.netlify.app

@benjie
Copy link
Copy Markdown
Member Author

benjie commented Jan 6, 2022

This has been updated again and is ready to merge 👍

@leebyron leebyron merged commit 7908822 into graphql:main Jan 6, 2022
@benjie benjie deleted the prettier branch January 6, 2022 22:09
@benjie benjie mentioned this pull request Apr 2, 2024
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leebyron leebyron Awaiting requested review from leebyron

Assignees

No one assigned

Labels

✏️ Editorial PR is non-normative or does not influence implementation 🐝 Process Related to Governance, Tools, or other meta work

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@benjie @leebyron