-
Notifications
You must be signed in to change notification settings - Fork 597
Improve apiref with experimental and ignored CRD description #4132
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
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rikatz The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/hold |
d081d55
to
2314d9b
Compare
d54a44e
to
9f25fb9
Compare
9f25fb9
to
47d554f
Compare
/hold cancel |
{{ range $type.Members -}} | ||
| `{{ .Name }}` _{{ markdownRenderType .Type }}_ {{- if contains "<gateway:experimental>" .Doc -}}<br /> :warning: **Experimental**{{ end -}}| {{ template "type_members" . }} | {{ markdownRenderDefault .Default }} | {{ range .Validation -}} {{ markdownRenderFieldDoc . }} <br />{{ end }} | | ||
{{ end -}} |
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.
tbh I think that Default and Validation could well live in the first cell of the row, under the Experimental warning, and then we can have only two columns, which should make the descriptions a lot more readable.
Maybe something like:
{{ range $type.Members -}} | |
| `{{ .Name }}` _{{ markdownRenderType .Type }}_ {{- if contains "<gateway:experimental>" .Doc -}}<br /> :warning: **Experimental**{{ end -}}| {{ template "type_members" . }} | {{ markdownRenderDefault .Default }} | {{ range .Validation -}} {{ markdownRenderFieldDoc . }} <br />{{ end }} | | |
{{ end -}} | |
{{ range $type.Members -}} | |
| `{{ .Name }}` _{{ markdownRenderType .Type }}_ {{- if contains "<gateway:experimental>" .Doc -}}<br /> :warning: **Experimental**{{ end -}}< br /> {{ markdownRenderDefault .Default }} {{ range .Validation -}} <br />{{ markdownRenderFieldDoc . }}{{ end }} | {{ template "type_members" . }} | | | | |
{{ end -}} |
Ideally, we can find a way to make the table take up more of the page as well, maybe by removing the in-page TOC?
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.
last famous words: seem simple. I have tried changing the defaults and validations to first field, but the misalignment caused by some validations that are huge are worst than the current situation.
The markdown tables are not really very configurable (https://squidfunk.github.io/mkdocs-material/reference/data-tables/#column-alignment) unless we do some CSS change, so I think changing the defaults and validations here will be harder.
Removing the TOC OTOH was doable and things got a bit more clear (I will push it soon)
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.
another approach I have tried was to move defaults and validations to a tooltip/annotation like, where when you hover over an information icon it would show each of them, but still the gain is not great on the Markdown tables.
We have fields that have a type of "LocalObjectReferenceWithNamespacesAndLasers" and rendering this still makes the first column be bigger than the second
Thanks for doing this @rikatz ! When I had a look at this, I had in my notes: Consider hiding the TOC using
in the frontmatter of the spec page. Which would remove the TOC on the right-hand side of this page, which would allow the tables to expand wider, and make the Descriptions easier to read. Alternatively, we could try and find a way to only have the TOC show the top-level objects or something (that is, Gateway, HTTPRoute, etc), rather than all the struct names. That's totally an improvement we can do later though. |
@youngnick We have an issue on the renderer that I have just realized, as soon as we add line breaks on godocs (to stick with a 90 column limit) it adds to the generated markdown table :/ So even extending the column for descriptions, we will have this weird situation where crd-ref-docs will add line breaks . I would need a bit more of time to think on how to deal with it (like change the gotemplate to remove the first line break of each line, but maybe this also turns the readability bad. I think in the end we will need to implement #4012 and then come back here to be sure that our API ref docs are assertive and readable. Anyway, I have removed the TOC and extended the table to 100% of its possible width, I guess it is a bit better now |
The current markers are helpful if I look up the API field in order to write out a yaml manifest. However, suppose I am browsing the documentation in order to determine whether Gateway API supports external auth as a standard feature. I might search for "external auth" or "externalauth", and then I might find the documentation for the ![]() The documentation for the data type is detailed and looks like the canonical documentation for the feature, and I might not think to search for the API field that uses the data type. As this seemingly canonical documentation does not mention the feature's experimental status, I might therefore conclude that the feature is not experimental, only to be surprised later when I start writing out a yaml manifest and look up the API field. Would it be easy and reasonable to add the "Experimental" marker in all places that document an experimental feature, or at least in the documentation for the data type? |
@Miciah yes, and to be honest I tried doing it. The problem is that the As an example, let's say you have a struct called
Then, in some future, you decide to add a new field like
See? The I can check if I can add on each |
@youngnick I don't wanna change this template more (not today), but we can bring the validation and defaulting down to these structs (now I kind of know how to do it). I am not sure it would make a lot of sense for someone to look into the api field, but need to click on it to see what validation is applied for each parent (eg.: you can have a MaxItems=1 for rules on TLSRoute, and a MaxItem=16 for HTTPRoute). I would probably like to see that on HTTPRoute fields, and not on the rules description But here we are :) |
The latest changes address my concern. Thank you! (You also fixed the formatting of the "appears in" list! 🎉!) |
Wow, #4132 (comment) is a huge improvement and addresses similar concerns I had in #1031, nice work @rikatz! |
What type of PR is this?
/kind documentation
What this PR does / why we need it:
This PR adds the following changes:
<gateway:experimental>
marker is detected<gateway:util:excludeFromCRD></gateway:util:excludeFromCRD>
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: