-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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
Markdown updates for 3.0.4 #3932
Conversation
Super special python script: from sys import argv
from pathlib import Path
import re
# this script tries to inflect the old links, some are just missing and we need to use the title's version instead
updates = {}
updates["revision-history"] = "appendix-a-revision-history"
updates["data-type-conversion"] = "appendix-b-data-type-conversion"
updates["using-r-f-c6570-implementations"] = "appendix-c-using-rfc6570-implementations"
updates["serializing-headers-and-cookies"] = "appendix-d-serializing-headers-and-cookies"
updates["percent-encoding-and-form-media-types"] = "appendix-e-percent-encoding-and-form-media-types"
updates["document-structure"] = "openapi-description-structure"
updates["oas-object"] = "openapi-object"
updates["components-security-schemes"] = "security-scheme-object"
updates["schema-composition"] = "composition-and-inheritance-polymorphism"
updates["http-codes"] = "http-status-codes"
updates["oas-document"] = "openapi-description"
updates["rich-text"] = "rich-text-formatting"
updates["relative-references"] = "relative-references-in-urls"
updates["runtime-expression"] = "runtime-expressions"
updates["runtime-expression-examples"] = "examples"
def kebab_it(c):
if c.lower() != c:
return f'-{c.lower()}'
return c
if __name__ == '__main__':
text = Path(argv[1]).read_text()
names = {}
removals = {}
for match in re.finditer(r'\n(.*)<a name="([^"]*)"', text):
name = match.group(2)
names[name] = ''.join([kebab_it(c) for c in name])
# was it a heading? file it for removal
if len(match.group(1)) and match.group(1)[0] == "#":
removals[name] = True
for current, replacement in names.items():
if replacement in updates:
replacement = updates[replacement]
text = text.replace(f'(#{current})', f'(#{replacement})')
# only remove if removal is indicated, otherwise update
if current in removals:
text = text.replace(f'<a name="{current}"></a>', '')
else:
text = text.replace(f'<a name="{current}"></a>', f'<a name="{replacement}"></a>')
print(text) Run it like: |
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.
A few quick first impressions...
versions/3.0.4.md
Outdated
* `binary` is used where unencoded binary data is allowed, such as when sending a binary payload as an HTTP message body, or as part of a `multipart/*` payload that allows binary parts | ||
* `byte` is used where binary data is embedded in a text-only format such as `application/json` or `application/x-www-form-urlencoded` | ||
- `binary` is used where unencoded binary data is allowed, such as when sending a binary payload as an HTTP message body, or as part of a `multipart/*` payload that allows binary parts | ||
- `byte` is used where binary data is embedded in a text-only format such as `application/json` or `application/x-www-form-urlencoded` |
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.
I'm a bit puzzled by this list indicator change. I feel like we have always consistently used *
for this and the change introduces a lot of noise. It's not a hill I want to die on, but I'd prefer *
(both because it's what I'm used to scaning visually for when I edit, and to trim down this diff)
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.
-
has evolved as a standard in markdown tools, the linter actually checks that the lists are consistent but I can totally enforce *
instead if that's our normal.
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.
-
has evolved as a standard in markdown tools
UGH why???? I'm a bit reluctant to start introducing digressions. I do think *
makes lists much easier to read in the source, but maybe that's because I'm more used to it. It's just... bullet points... *
looks like a bullet point, -
does not!
Source | Target | Alternative | ||
------ | ------ | ----------- | ||
[Security Requirement Object](#securityRequirementObject) `{name}` | [Security Scheme Object](#securitySchemeObject) name under the [Components Object](#componentsObject) | _n/a_ | ||
[Discriminator Object](#discriminatorObject) `mapping` _(implicit, or explicit name syntax)_ | [Schema Object](#schemaObject) name under the Components Object | `mapping` _(explicit URI syntax)_ | ||
[Operation Object](#operationObject) `tags` | [Tag Object](#tagObject) `name` (in the Components Object) | _n/a_ | ||
[Link Object](#linkObject) `operationId` | [Path Item Object](#pathItemObject) `operationId` | `operationRef` | ||
| Source | Target | Alternative | | ||
| -------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------- | --------------------------------- | | ||
| [Security Requirement Object](#security-requirement-object) `{name}` | [Security Scheme Object](#security-scheme-object) name under the [Components Object](#components-object) | _n/a_ | | ||
| [Discriminator Object](#discriminator-object) `mapping` _(implicit, or explicit name syntax)_ | [Schema Object](#schema-object) name under the Components Object | `mapping` _(explicit URI syntax)_ | | ||
| [Operation Object](#operation-object) `tags` | [Tag Object](#tag-object) `name` (in the Components Object) | _n/a_ | | ||
| [Link Object](#link-object) `operationId` | [Path Item Object](#path-item-object) `operationId` | `operationRef` | | ||
|
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.
I have mixed feelings about this sort of table re-formatting. On the one hand, it appeals to my sense of aesthetics. On the other, it's very hard to get people to maintain it correctly. I've gotten rather used to not bothering with consistent widths most of the time. But I'm sure I bothered with it at least once, so... idk.
For me, it boils down to: "If a new contributor messes up this table spacing, will we block their PR until they fix it, and is that the sort of contributor experience we want to create?"
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.
Although if this is being done by prettier
or some other script that we can set up to run as a pre-commit hook, then I'm 100% fine with it (and even the list item change, although I really do find *
much easier to spot when scanning text, as it is rarely used for anything other than lists, while -
is used for many, many things and has less visual weight as weel).
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.
it is all automatic, a couple of tables had varying numbers of columns but otherwise this is all auto-fix and we can get pre-commit and/or CI to do it or at least check it
@mikekistler we've said the HTML rendering is authoritative now, so yes. (but it's a worthwhile question!) |
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.
I have gone through the entire PR ! Formatting looks good. Nice @lornajane !!
About the TOC, if I understood the thread correctly, respec builds a TOC as well, so we do not need one in our source at all |
e360e30
to
abeb7f3
Compare
I'm not sure respec does the table of contents in a way we can use, so I've regenerated it in this update. Thanks @ralfhandl for getting into the respec details, I hadn't got there yet! Changed items:
It did occur to me that we could keep the |
Our HTML build script removes the table of contents from the Markdown, the ToC we see for example in https://spec.openapis.org/oas/latest.html is generated by ReSpec from the section headlines. Here's the relevant part of our build script: OpenAPI-Specification/scripts/md2html/md2html.js Lines 176 to 178 in 6bd37a3
Every line between lines starting with With the current change from A better way forward would be to completely remove the "Table of Contents" subsection because
|
versions/3.0.4.md
Outdated
@@ -17,80 +17,35 @@ For examples of OpenAPI usage and additional documentation, please visit [learn. | |||
For extension registries and other specifications published by the OpenAPI Initiative, as well as the authoritative rendering of this specification, please visit [spec.openapis.org](https://spec.openapis.org/). | |||
|
|||
## Table of Contents |
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.
Remove the heading
Should also tackle #1720 |
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.
Looks good. 👍
I reviewed this by performing all the automated changes myself on a fresh branch and then reviewing the (much smaller) diff.
Thank you @lornajane for this work.
My notes on the process that @lornajane put in the PR description: get checkout v3.0.4-dev
npx prettier --write --single-quote 3.0.4.md
Create markdown-lint.yaml with contents given, thennpx markdownlint-cli --config markdown-lint.yaml --fix 3.0.4.md
awk '/## Table of Contents/{f=1} //{f=0; print ""; next} {if (f==0) {print}} ' 3.0.4.md > temp.md; mv temp.md 3.0.4.md
python kebab_it.py 3.0.4.md > temp.md; mv temp.md 3.0.4.md
npx markdownlint-cli --config markdown-lint.yaml --fix 3.0.4.md |
- heading levels aren't continuous MD001 - code fences need a language MD040 - table has the wrong number of cells MD056
Updated to apply the changes to the newest 3.0.4 version, and marked as ready to review as I don't think we have any more changes in flight for this branch. |
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.
Renewing my approval. 👍
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.
Also checked the generated HTML:
- internal links use the new anchors
- all "code" blocks are prettier-formatted
- no further differences (after fixing the build script 😁)
@@ -1,6 +1,6 @@ | |||
# OpenAPI Specification | |||
|
|||
#### Version 3.0.4 | |||
## Version 3.0.4 |
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.
This threw off the respec build script, fixed with #3992
We need to handle the changes to anchor links and headings (see #3548 for context). We also have #3596 to add tooling, which I will work on after we've done this initial cleanup.
The commits tell their own story, but this is all very repeatable and hopefully transferrable between branches. Shout out to @handrews who got me started with the anchor/link rewriting script.
The process goes like this:
Run prettier for formatting
prettier --write --single-quote 3.0.4.md
Run markdownlint to fix whatever it can
markdownlint --fix 3.0.4.md
.markdownlint.yaml
:Manually fix additional markdownlint problems
Take out the table of contents, we don't need it since both GitHub and respec render these automatically
Run a magical one-off script to update/fix/rewrite/remove all our anchors and internal links. Basic idea:
differently or we were using an anchor link that didn't match the
title
It's 850 lines of change, I don't know how we're going to review it, but take a look!