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

Formatting: use markdown lists in 'one of' lists #726

Merged
merged 2 commits into from
Apr 1, 2021

Conversation

benjie
Copy link
Member

@benjie benjie commented Jun 2, 2020

spec-md recently added support for bulleted "one of" productions; this PR leverages this features to make the "one of" productions render similarly in regular markdown parsers (such as GitHub's) as they do in spec-md.

This is a precursor to formatting the spec with prettier (since, without this formatting, prettier will remove these "significant" line breaks).


Originally attempted with Markdown linebreaks, but there was significant pushback. These whitespace changes make the one of lists render similarly in regular markdown parsers (such as GitHub's) as they do in spec-md.

@IvanGoncharov IvanGoncharov added the 🐝 Process Related to Governance, Tools, or other meta work label Jun 2, 2020
@IvanGoncharov
Copy link
Member

@benjie I tried to run prettier on spec couple years ago and reported few issues:
https://github.com/prettier/prettier/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc+author%3AIvanGoncharov+label%3Alang%3Amarkdown

I really like to use prettier but I don't want to add brittle and non obvious workarounds.
What I propose is to change spec-md to use code blocks for grammar rules:
image
Since I think it a generally bad idea to allow prettier to run on grammar rules.

@benjie
Copy link
Member Author

benjie commented Jun 2, 2020

@IvanGoncharov I saw your eariler work, that's what inspired me to put the time in. I now have prettier running fully functionally against the GraphQL spec with a very tight and carefully reviewed diff thanks to some changes to spec-md.

Whether or not those changes get merged, this change helps to ensure the code is output in a similar way through Markdown as it is through spec-md, which is one of the goals of spec-md, I think, and makes the spec source easier to view.

I challenge the assertion that this is a "brittle and non obvious workaround"; this is part of the original Markdown spec:

When you do want to insert a <br /> break tag using Markdown, you end a line with two or more spaces, then type return.

And is supported by all markdown processors I know of, including prettier. It's standard markdown formatting.

@IvanGoncharov
Copy link
Member

I challenge the assertion that this is a "brittle and non obvious workaround";

It not visible in editors, for example here is your file in GitHub editor:
image
So how we can expect people to figure it out from a first try?

I still think grammar shouldn't be written in a pure markdown.
Can we trust that prettier doesn't change the current behavior in some other way that will break grammar?

@benjie
Copy link
Member Author

benjie commented Jun 2, 2020

Can we trust that prettier doesn't change the current behavior in some other way that will break grammar?

Yes. I've literally diffed the output HTML and it's identical except for a few small changes unrelated to grammar - you can read some about it in #727.

However this PR isn't specifically about prettier, just like #725 wasn't. I believe it has value in its own right - it is a formatting concern rather than a grammar concern.

To me it's a clear enhancement to the GraphQL spec source code -

Spec output:

Screenshot_20200602_151412

Spec text in markdown before this change:

Screenshot_20200602_151521

Spec text in markdown after this change:

Screenshot_20200602_151633

This last screenshot is clearly a lot closer to the intent. And the HTML generated by spec-md is byte-for-byte identical before and after this change. It's a small change that gives a significant enhancement at virtually zero cost.

@IvanGoncharov
Copy link
Member

Can we trust that prettier doesn't change the current behavior in some other way that will break grammar?

Yes. I've literally diffed the output HTML and it's identical except for a few small changes unrelated to grammar - you can read some about it in #727.

I don't mean at the moment I meant that I worried about prettier changing formating in future releases.

As for the readability, I think code block is the closes option because it keeps indentation:

Letter :: one of  
  `A` `B` `C` `D` `E` `F` `G` `H` `I` `J` `K` `L` `M`  
  `N` `O` `P` `Q` `R` `S` `T` `U` `V` `W` `X` `Y` `Z`  
  `a` `b` `c` `d` `e` `f` `g` `h` `i` `j` `k` `l` `m`  
  `n` `o` `p` `q` `r` `s` `t` `u` `v` `w` `x` `y` `z`

And for readability inside PRs, I personally don't want to review changes where I need to pay attention to the trailing spaces.

Also, in the long run, we probably want to run some markdown linter (e.g. Vale) and we will have the same situation because it can't distinguish syntax rules from the rest of the text.

@IvanGoncharov
Copy link
Member

@benjie Also what about writing experience as most of the editors don't show trailing spaces as I showed in the screenshot?
#726 (comment)

@IvanGoncharov
Copy link
Member

@benjie Pragmatically speaking without changes to spec-md your current solution is the best one.
So we need to find out if such changes in line with spec-md design and would be accepted.

@leebyron Can you please comment on what do you think is the best solution?

@benjie
Copy link
Member Author

benjie commented Jun 2, 2020

I hear your concerns about whitespace, but it's a technical spec so I'm sure technical people can be disciplined enough to be careful with whitespace changes. When reviewing a diff, whitespace changes are shown in GitHub:

Screenshot_20200602_181419

When editing, whitespace can be seen via text selection:

Screenshot_20200602_181433

I don't mean at the moment I meant that I worried about prettier changing formating in future releases.

Prettier, as far as I know, won't deliberately break the markdown rules (of which this is a fundamental one, defined before even headers are defined); but if it does then we can pin an earlier version.

As for the readability, I think code block is the closes option because it keeps indentation [...] Pragmatically speaking without changes to spec-md your current solution is the best one. So we need to find out if such changes in line with spec-md design and would be accepted.

I personally prefer the more markdown-native approach over the codeblock escape-hatch, but you're much more familar with the GraphQL spec than I. Formatted productions like this seem to be a minority case, most productions can wrap happily* without any change to the output HTML. As you say, it's up to @leebyron to state what he wants for spec-md.

  • with my changes to spec-md which enable text to wrap

leebyron added a commit to leebyron/spec-md that referenced this pull request Jan 10, 2021
To help resolve the issue raised in graphql/graphql-spec#726, this allows each line of a grid of "one of" tokens to start with a line bullet.

The spec-md rendered results are equivalent, however in other tools the content should be much easier to read.
leebyron added a commit to leebyron/spec-md that referenced this pull request Jan 10, 2021
To help resolve the issue raised in graphql/graphql-spec#726, this allows each line of a grid of "one of" tokens to start with a line bullet.

The spec-md rendered results are equivalent, however in other tools the content should be much easier to read.
leebyron added a commit to leebyron/spec-md that referenced this pull request Jan 10, 2021
To help resolve the issue raised in graphql/graphql-spec#726, this allows each line of a grid of "one of" tokens to start with a line bullet.

The spec-md rendered results are equivalent, however in other tools the content should be much easier to read.
leebyron added a commit to leebyron/spec-md that referenced this pull request Jan 10, 2021
To help resolve the issue raised in graphql/graphql-spec#726, this allows each line of a grid of "one of" tokens to start with a line bullet.

The spec-md rendered results are equivalent, however in other tools the content should be much easier to read.
@leebyron
Copy link
Collaborator

I know we discussed this in person months ago, but for posterity (now that the solution is up as part of a spec-md release):

While trailing space line breaks are officially part of markdown, it's one of my very least favorite features for the reasons Ivan raised. Many tools and linters actually highlight trailing spaces as an error or editors that simply remove them (mine strips trailing whitespace on file save).

The fix as part of spec-md allows gridded one of productions to optionally use a bulleted list. It's not an ideal solution since the bullets could imply each row is its own entity which might be slightly confusing in raw markdown view, but the spec rendered result is the same. @benjie if you're up for reviving this one to use bullet lists instead, I think that will solve

Base automatically changed from master to main February 3, 2021 04:50
@benjie benjie changed the title Use markdown line breaks in 'one of' lists Formatting: use markdown lists in 'one of' lists Feb 8, 2021
@benjie
Copy link
Member Author

benjie commented Feb 8, 2021

@leebyron Done; and have confirmed with diff that the spec renders identically (byte for byte) on main as on this branch. This should be suitable for merging 👍

@benjie
Copy link
Member Author

benjie commented Feb 8, 2021

(Musing: I bet if Markdown had used [space][backslash] or similar for their linebreak syntax rather than [space][space] this discussion would have gone a different direction.)

@leebyron
Copy link
Collaborator

leebyron commented Apr 1, 2021

Nice! Happy to see this prettier-ready

@leebyron leebyron merged commit bed0a19 into graphql:main Apr 1, 2021
@leebyron leebyron added the ✏️ Editorial PR is non-normative or does not influence implementation label Apr 1, 2021
@benjie benjie deleted the format-one-ofs branch April 1, 2021 19:03
@leebyron leebyron added this to the May2021 milestone Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✏️ Editorial PR is non-normative or does not influence implementation 🐝 Process Related to Governance, Tools, or other meta work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants