Make an editorial pass over the API documentation. #509

jpeach · 2020-12-20T01:56:45Z

What type of PR is this?

/kind cleanup
/kind documentation

What this PR does / why we need it:

Especially now that enum constants are rendered in the HTML API
documentation, the godoc conventions make it harder for a reader
to understand the intentions of the documentation.

Make an editorial pass that improves readability without any semantic
changes.

Which issue(s) this PR fixes:

n/a

Does this PR introduce a user-facing change?:

NONE

apis/v1alpha1/tlsroute_types.go

youngnick · 2020-12-20T23:01:16Z

/lgtm

youngnick · 2020-12-20T23:02:30Z

Looks like you'll need to revert some changes to pass go linting though, godoc requires constant comments to be phrased CONSTANTNAME does something.

youngnick · 2020-12-20T23:04:12Z

/lgtm cancel

jpeach · 2020-12-20T23:45:22Z

On 21 Dec 2020, at 10:02 am, Nick Young ***@***.***> wrote: Looks like you'll need to revert some changes to pass go linting though, godoc requires constant comments to be phrased CONSTANTNAME does something.

Yup, I pinged slack on this. The go convention conflicts with making the docs useful for their intended purpose (people generating YAML). Specifically, the constant names are completely useless to anyone reading the docs to figure out how to interpret the API, since those names aren't visible in the API. We could nobble the golint error if we switch to golangci-lint.

robscott · 2020-12-21T20:02:08Z

Thanks for the work on this @jpeach! There are a lot of great improvements here, I'm not quite sure what to do as far as doc links though. The links added in this PR would look somewhat out of place everywhere but the API spec docs. This is likely much more complicated, but it seems like this is the kind of issue that would ideally be solved by improving the docs/spec generation here. If that could recognize the names of types it was processing and add links automatically we'd have a readable spec + go types that pass linting.

jpeach · 2020-12-22T00:03:16Z

Thanks for the work on this @jpeach! There are a lot of great improvements here, I'm not quite sure what to do as far as doc links though. The links added in this PR would look somewhat out of place everywhere but the API spec docs. This is likely much more complicated, but it seems like this is the kind of issue that would ideally be solved by improving the docs/spec generation here. If that could recognize the names of types it was processing and add links automatically we'd have a readable spec + go types that pass linting.

Yeah IIUC godoc automatically links type names that it finds in the doc text. But the godoc currently has a ton of ugliness, so I don't really see the markdown links as a blocker here. My perspective is that most people will read and learn the API through the HTML docs.

ahmetb/gen-crd-api-reference-docs#30

robscott · 2020-12-22T02:19:41Z

@jpeach that is a good point, the annotations we already have make things like https://pkg.go.dev/sigs.k8s.io/service-apis/apis/v1alpha1 rather ugly. This would represent a further shift away from readable godocs towards more readable HTML docs. I'm not sure that's necessarily a bad thing, but interested in what others think here.

/cc @hbagdi @danehans @bowei

hbagdi · 2020-12-28T18:36:06Z

My perspective is that most people will read and learn the API through the HTML docs.

I think we have control over this part of user-experience. Most doc generators have some quirks that make them harder to use. Whenever I'm browsing upstream k8s APIs, I use the HTML browser to figure out the types I need to lookup and then go and read the godoc version.

Regarding the markdown in Godoc, I'd rather prefer to not have it in there but I can live with a little bit of it sprinkled around.

hbagdi · 2020-12-28T18:38:41Z

apis/v1alpha1/gateway_types.go

+	// An opaque identifier that represents a specific IP address. The interpretation
+	// of the name is dependent on the controller. For example,
+	// a "NamedAddress" might be a cloud-dependent ID for a "static"
+	// or "elastic" IP.


I think it is best to leave the idiomatic Godoc style " is ..." in here for the sake of consistency across the entire project.

The problem with the Go idiom here is that the API isn't the constant t name but the constant value (ie. the value is what you need in the YAML). So using the constant name here is actually misleading readers of the documentation. It's also confusing because the constant names done appear anywhere in the HTML rendering.

This is an interesting one. I took a look at the Kubernetes reference docs and they lack consistency here but the godoc style Harry is suggesting above does seem to be more common (ConfigMap is an example of that). Your proposed change does certainly read better in the reference docs, we just need to determine how heavily we want to prioritize that over other formats.

Here's an example of the current docs:

Looking at "NamedAddress", which is a designated enum value that API clients have to provide, it is described in terms of a "NamedAddressType", a term which is never defined in the documentation and is utterly irrelevant to anyone who doesn't have the Go code in front of them.

In terms of the API, the fact that this constant is available in Go doesn't really matter. What API consumers need to know is the value of the constant.

robscott

Thanks for your work on this @jpeach! Lots of great cleanup + godoc improvements here. I'd personally prefer to separate out the markdown link additions/discussion so we can get the rest of this cleanup in sooner.

robscott · 2021-01-13T05:37:12Z

apis/v1alpha1/gateway_types.go

+	// An opaque identifier that represents a specific IP address. The interpretation
+	// of the name is dependent on the controller. For example,
+	// a "NamedAddress" might be a cloud-dependent ID for a "static"
+	// or "elastic" IP.


This is an interesting one. I took a look at the Kubernetes reference docs and they lack consistency here but the godoc style Harry is suggesting above does seem to be more common (ConfigMap is an example of that). Your proposed change does certainly read better in the reference docs, we just need to determine how heavily we want to prioritize that over other formats.

apis/v1alpha1/gateway_types.go

apis/v1alpha1/gatewayclass_types.go

apis/v1alpha1/tlsroute_types.go

robscott

Thanks for the work on this! A few more questions and nits, but lots of good improvements here. Still not sure about the transition away from the more traditional Type is ... structure. Added something to agenda tomorrow to try to get some consensus on which structure we want to standardize on throughout and how much we want to prioritize the readability of reference docs.

apis/v1alpha1/gateway_types.go

robscott · 2021-01-20T04:09:30Z

apis/v1alpha1/gateway_types.go

-	// Value. Examples: "1.2.3.4", "128::1", "my-ip-address". Validity of the
-	// values will depend on `Type` and support by the controller.
+	// Value of the address. The validity of the values will depend
+	// on the type and support by the controller.


Nit: I think the backticks and capitalization above made it more clear that "type" was referring to an API field and not a characteristic of the controller.

That would be find if it was used consistently. We can make a separate pass to add that.

apis/v1alpha1/gateway_types.go

apis/v1alpha1/httproute_types.go

hack/verify-golint.sh

danehans · 2021-01-20T19:59:48Z

IMO we should prioritize documentation for godocs. Is it possible for this PR to focus on changes that are applicable to godoc and html? The more controversial changes can be discussed further in a separate PR.

hbagdi · 2021-01-21T06:13:03Z

I think the minor intrusive changes are okay but the change of * to &ast; is taking a step too further as it makes godocs incomprehensible.

robscott · 2021-02-03T20:37:00Z

Hey @jpeach, we discussed this at the community meeting today - here's a quick recap:

We want to ensure that godocs remain readable and accurate
We love the API reference docs and want to find a way to update the generator to escape special characters
If we can't find a way to update the generator, we may need to evaluate using https://pkg.go.dev/sigs.k8s.io/service-apis/apis/v1alpha1 temporarily until we can find a better solution

I think there's general agreement that the best case scenario involves finding a way to update the generator so we can have both readable godocs and great reference docs. Happy to help figure that out, just not sure where to even start.

k8s-ci-robot · 2021-02-14T00:12:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpeach

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [jpeach]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Especially now that enum constants are rendered in the HTML API documentation, the godoc conventions make it harder for a reader to understand the intentions of the documentation. To handle passing a literal "*" character through the two layers of Markdown, we postprocess the`"*"` pattern into a HTML entity. Make an editorial pass that improves readability without any semantic changes. Signed-off-by: James Peach <jpeach@vmware.com>

jpeach · 2021-02-14T00:46:53Z

/retest

robscott

Thanks for all your work on this! This LGTM, just needs one more rebase.

robscott · 2021-02-16T03:03:58Z

hack/api-docs/generate.sh

@@ -50,4 +50,4 @@ fi
 gendoc::build
 gendoc::exec \
    -api-dir sigs.k8s.io/gateway-api/apis/v1alpha1 \
-    -out-file "$1"
+    -out-file /dev/stdout | sed '-es|&quot;*\*|\&quot;\&ast;|g' > "$1"


With #549 merged I don't think this and the associated changes to escaping asterisks are necessary anymore. I think it's still worth looking into ways to improve our docs generation, but what we have now is probably good enough for the v0.2.0 release.

hbagdi · 2021-02-16T20:00:42Z

#552 merged instead
/close

k8s-ci-robot · 2021-02-16T20:00:48Z

@hbagdi: Closed this PR.

In response to this:

#552 merged instead
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2021-02-16T20:00:49Z

@jpeach: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 20, 2020

k8s-ci-robot requested review from robscott and thockin December 20, 2020 01:56

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 20, 2020

youngnick reviewed Dec 20, 2020

View reviewed changes

apis/v1alpha1/tlsroute_types.go Outdated Show resolved Hide resolved

k8s-ci-robot assigned youngnick Dec 20, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 20, 2020

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 20, 2020

k8s-ci-robot requested review from bowei, danehans and hbagdi December 22, 2020 02:19

jpeach force-pushed the docs-editorial-pass branch from 012b8e2 to a1f759f Compare December 23, 2020 00:16

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 23, 2020

hbagdi reviewed Dec 28, 2020

View reviewed changes

robscott reviewed Jan 13, 2021

View reviewed changes

jpeach force-pushed the docs-editorial-pass branch from a1f759f to 2a0cfe3 Compare January 17, 2021 02:41

jpeach changed the title ~~WIP: make an editorial pass over the API documentation~~ Make an editorial pass over the API documentation. Jan 17, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 17, 2021

jpeach force-pushed the docs-editorial-pass branch from 2a0cfe3 to 7245c26 Compare January 17, 2021 02:44

jpeach force-pushed the docs-editorial-pass branch from 7245c26 to d8c0929 Compare January 17, 2021 03:04

robscott reviewed Jan 20, 2021

View reviewed changes

jpeach force-pushed the docs-editorial-pass branch from d8c0929 to d38c202 Compare January 20, 2021 05:06

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 29, 2021

jberger mentioned this pull request Feb 8, 2021

Rendering of literal star characters in documentation #539

Closed

hbagdi added this to the v0.2.0 milestone Feb 10, 2021

jpeach force-pushed the docs-editorial-pass branch from d38c202 to 74649fc Compare February 14, 2021 00:12

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2021

jpeach force-pushed the docs-editorial-pass branch from 74649fc to 52bf5e2 Compare February 14, 2021 00:38

robscott reviewed Feb 16, 2021

View reviewed changes

robscott mentioned this pull request Feb 16, 2021

Make an editorial pass over the API documentation. #552

Merged

k8s-ci-robot closed this Feb 16, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make an editorial pass over the API documentation. #509

Make an editorial pass over the API documentation. #509

jpeach commented Dec 20, 2020 •

edited

Loading

youngnick commented Dec 20, 2020

youngnick commented Dec 20, 2020

youngnick commented Dec 20, 2020

jpeach commented Dec 20, 2020 via email

robscott commented Dec 21, 2020

jpeach commented Dec 22, 2020

robscott commented Dec 22, 2020

hbagdi commented Dec 28, 2020

hbagdi Dec 28, 2020

jpeach Dec 28, 2020

robscott Jan 13, 2021

jpeach Jan 17, 2021

robscott left a comment

robscott Jan 13, 2021

robscott left a comment

robscott Jan 20, 2021

jpeach Feb 14, 2021

danehans commented Jan 20, 2021

hbagdi commented Jan 21, 2021

robscott commented Feb 3, 2021

k8s-ci-robot commented Feb 14, 2021

jpeach commented Feb 14, 2021

robscott left a comment

robscott Feb 16, 2021

hbagdi commented Feb 16, 2021

k8s-ci-robot commented Feb 16, 2021

k8s-ci-robot commented Feb 16, 2021

Make an editorial pass over the API documentation. #509

Make an editorial pass over the API documentation. #509

Conversation

jpeach commented Dec 20, 2020 • edited Loading

youngnick commented Dec 20, 2020

youngnick commented Dec 20, 2020

youngnick commented Dec 20, 2020

jpeach commented Dec 20, 2020 via email

robscott commented Dec 21, 2020

jpeach commented Dec 22, 2020

robscott commented Dec 22, 2020

hbagdi commented Dec 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danehans commented Jan 20, 2021

hbagdi commented Jan 21, 2021

robscott commented Feb 3, 2021

k8s-ci-robot commented Feb 14, 2021

jpeach commented Feb 14, 2021

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbagdi commented Feb 16, 2021

k8s-ci-robot commented Feb 16, 2021

k8s-ci-robot commented Feb 16, 2021

jpeach commented Dec 20, 2020 •

edited

Loading