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

revise style guidelines for capitalization #24645

Merged
merged 1 commit into from
Feb 7, 2021

Conversation

geoffcline
Copy link
Contributor

I'd like to start a conversation about the style guide and capitalization, based on the conversation in https://github.com/kubernetes/website/pull/24077/files,

Specifically, this comment from @wabernatScality .

Currently, the style guide explicitly prohibits usage of "object" -- such as "Pod object" or "PodList object"

Judicious use of "object" could improve clarity. It's a hint to the reader that the distinction of being an API object is important.

Right now, the guidelines are overly reliant on capitalization to allow readers to make that distinction. Capitalization is a weak way to signal information to the reader. This information may be lost entirely due to assistive technology, differences in visual abilities, or translation.

I look forward to the feedback :)

@k8s-ci-robot k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 19, 2020
@k8s-ci-robot k8s-ci-robot added language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Oct 19, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Oct 19, 2020
@netlify
Copy link

netlify bot commented Oct 19, 2020

Deploy preview for kubernetes-io-master-staging ready!

Built with commit 3c4ca94

https://deploy-preview-24645--kubernetes-io-master-staging.netlify.app

@netlify
Copy link

netlify bot commented Oct 19, 2020

Deploy preview for kubernetes-io-master-staging ready!

Built with commit 96c4813

https://deploy-preview-24645--kubernetes-io-master-staging.netlify.app

@tengqm
Copy link
Contributor

tengqm commented Oct 20, 2020

Judicious use of "object" could improve clarity. It's a hint to the reader that the distinction of being an API object is important.

I agree.

Capitalization is a weak way to signal information to the reader.

I agree. Maybe we should instead enforce code style formatting to API resource, command line tools, resource field names etc.

@sftim
Copy link
Contributor

sftim commented Oct 21, 2020

Hi @geoffcline

This kind of change is best discussed first in an issue - and then might get further brought up in a Kubernetes SIG Docs weekly meeting.

Would you be willing to open an issue to start that discussion? I'm going to
/hold
this change pending that discussion happening.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 21, 2020
@tengqm
Copy link
Contributor

tengqm commented Oct 22, 2020

Yes. We do need a dedicated issue (or even better with real examples) for this discussion.
Maybe we can use this PR as an example for the discussion. A separate issue without examples always looks like empty talks.

@geoffcline
Copy link
Contributor Author

I brought up this issue for discussion in the SIG docs planning meeting today (21OCT2020).

The general consensus was approval for this specific proposal, and we didn't feel that extensive discussion was necessary.

I'd like to keep the discussion in this PR, and scoped to the specific change I'm suggesting.

Let's have a bias for action ✔

@sftim
Copy link
Contributor

sftim commented Oct 23, 2020

Unholding. With some tweaks this can be good to merge.
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 23, 2020
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Some suggestions

content/en/docs/contribute/style/style-guide.md Outdated Show resolved Hide resolved
content/en/docs/contribute/style/style-guide.md Outdated Show resolved Hide resolved
content/en/docs/contribute/style/style-guide.md Outdated Show resolved Hide resolved

{{< table caption = "Do and Don't - API objects" >}}
Do | Don't
:--| :-----
The pod has two containers. | The Pod has two containers.
The HorizontalPodAutoscaler is responsible for ... | The HorizontalPodAutoscaler object is responsible for ...
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest

Suggested change
The HorizontalPodAutoscaler is responsible for ... | The HorizontalPodAutoscaler object is responsible for ...
The `HorizontalPodAutoscaler` is responsible for ... | The HorizontalPodAutoscaler object is responsible for ...

(and keep the line)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had purposely left out code style from the do/don't table, because I was wary of trying to illustrate two concepts at once.

I'm leaning toward using code style in the table, but I will add a link to the code style guidelines in the preceding paragraph.

Choose a reason for hiding this comment

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

Geoff has the instinct of correct. In the present example, "HorizontalPodAutoscaler" should be set only in code (in all examples) or not in code at all. Mixing them up only confuses two issues, and the reader. A brief spleen-dump concerning code formatting follows.

Once upon a time, monospace type was used to indicate command-line interactions and code snippets, full stop. Your guide now recommends it for paths, command tools, and component names. The present example adds Kubernetes objects to this list. This is a mistake: It feels like you're making a bright distinction, because you already know what you're trying to tell the reader, and putting it in monospace makes it stand out and look like documentation. In the present example, Pascal case already conveys quite clearly that the word HorizontalPodAutoscaler is an object, or at least is not an ordinary word (are objects considered components? Who adjudicates this?) Setting it in monospace adds no new meaning and burdens your writers and readers with another exceptional rule. As soon as someone copies and pastes the passage, the monospace signifier vanishes, leaving you where you started.

As @tengqm pointed out earlier, capitalization is a weak way of conveying information. Modifying typesetting is even weaker, especially when rules—however well-intentioned they may be—require its use to proliferate. (If every word is bold, no word is bold...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a good point about copy/paste.

I don't want to wade too deep into the code style debate here. The new draft page provides flexibility, and I think is a good next step.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't want to wade too deep into the code style debate here.

This PR should IMO either revise the code style recommendations or fit in with them. I'm not so keen on adding recommendations that conflict with other parts of the same page.

The easy option is to follow the current guidance on code style, and then if this merges come back and tweak it. It's the same idea as keeping PRs small that change source code: many small PRs that make incremental improvements, rather than one large change set that aims for perfection.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that the style guide now offers more styling options for API objects.

  • Do you need to show code styling in this section or could you state that the examples focus
    on capitalization and usage of general terms?
  • Is code styling of API objects optional? Is it acceptable to use code styling of a Kubernetes
    resource term that also adds "resource" or "object" after the term? Probably not.

Again, this section comments on how the API resource names/definitions are capitalized (which
matches how the name is used in the specification). This section also adds that it is acceptable to
add the word "object" after the term. Thirdly, the section adds that at times it is acceptable to use a general term (with sentence case) to refer to Kubernetes related software objects such as pod, node, secret, service, container.

content/en/docs/contribute/style/style-guide.md Outdated Show resolved Hide resolved
leads to an awkward construction.
There is a subtle distinction between formal API objects and general concepts. Use "object" where appropriate to clarify the meaning of terms. Calling out a "Pod object" may be appropriate when moving from general concepts to specific API objects. For example, "Pods may have multiple volume mounts. The Pod object includes a Volumes field."

Do not use "object" repetitively, such as where it's clear a term is an API object.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hello @geoffcline , Some feedback:
Good idea to relax the use of "object" with resource types. If in doubt, I think it is a good idea
to log an issue to propose a change (not everyone is able to attend a meeting).
Note: The current guidance was probably added to curb overuse.

nit: I am tempted to drop line 54.
Does this suggest that the content should introduce an API term with "object"
and then drop the use of "object" with further instances of the term within a page?
I don't know whether the guide needs to go into this level of detail on the usage of "object".
What do you think about adding:
You may use the word "resource", "API", or "object" to clarify a Kubernetes resource type in a sentence. Note that Kubernetes resource names are capitalized. However, capitalization alone is not a reliable way to identify Kubernetes resource types in a sentence.

Regarding the statement about "weak signal and lost due to ...", this seems to be referring to
the same possible issue of using code formatting for resource types?
I can't tell whether this type of visual hint to the reader is helpful. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like your wording on clarifying resource types, thanks.

I agree about the similarities between capitalization and code style. I do think code style sends a bit clearer signal, and is a better way to indicate something is a string used in code (and thus shouldn't be translated).

I lean towards offering people more options, but I understand we may want the style guidelines to be more definitive.

I'm optimistic my next draft will address these questions.

@geoffcline
Copy link
Contributor Author

thank you all for your feedback and comments! I will review and push an update.

@geoffcline
Copy link
Contributor Author

I pushed a revised copy of the guidance, thanks!

@geoffcline
Copy link
Contributor Author

Do we need to discuss how to handle contradictions between honoring the capitalization in the API reference and using UpperCamelCase when documenting API objects?

@sftim
Copy link
Contributor

sftim commented Oct 28, 2020

Do we need to discuss how to handle contradictions between honoring the capitalization in the API reference and using UpperCamelCase when documenting API objects?

Can you give an example? API object names (eg ConfigMap) are case insensitive. If you see that written as configmap it's still OK to call that a ConfigMap instead.

@geoffcline
Copy link
Contributor Author

API object names (eg ConfigMap) are case insensitive. If you see that written as configmap it's still OK to call that a ConfigMap instead

this is the info I was looking for

@geoffcline
Copy link
Contributor Author

Here is a link to the recently revised page

https://deploy-preview-24645--kubernetes-io-master-staging.netlify.app/docs/contribute/style/style-guide/

Feedback appreciated!

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

LGTM label has been added.

Git tree hash: 9df858e7d006097ebe992776bb1816dbc6bdbcd9

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 9, 2020
@sftim
Copy link
Contributor

sftim commented Dec 9, 2020

BTW this is relevant to #20862

The two ContainerStateTerminated objects ... | The two ContainerStateTerminateds ...
The `Volume` object contains a `hostPath` field. | The volume object contains a hostPath field.
Every `ConfigMap` is part of a namespace. | Every configMap is part of a namespace.
For managing confidential data, consider using the Secret API. | For managing confidential data, consider using a secret.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not wrap "Secret" with backtiqs?
Just trying to make sure the style is consistent... we are surrounding "ConfigMap" with backtiqs on line 63.

Looks good to me other than this "inconsistency".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to illustrate different options in the do/don't table, such as backticks or saying "the Secret API".

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay I see. That makes sense.

@tengqm
Copy link
Contributor

tengqm commented Dec 11, 2020

/lgtm

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

LGTM label has been added.

Git tree hash: e5285ec8b3c4eac23166ee1765ed2790e026734b

@geoffcline
Copy link
Contributor Author

@kbhawkey do you mind giving me another LGTM?

@@ -44,22 +44,24 @@ The English-language documentation uses U.S. English spelling and grammar.

### Use upper camel case for API objects

When you refer specifically to interacting with an API object, use [UpperCamelCase](https://en.wikipedia.org/wiki/Camel_case), also known as Pascal Case. When you are generally discussing an API object, use [sentence-style capitalization](https://docs.microsoft.com/en-us/style-guide/text-formatting/using-type/use-sentence-style-capitalization).
When you refer specifically to interacting with an API object, use [UpperCamelCase](https://en.wikipedia.org/wiki/Camel_case), also known as Pascal case. You may see different capitalization, such as "configMap", in the [API Reference](/docs/reference/generated/kubernetes-api/{{< param "version" >}}/). When writing general documentation, it's better to use upper camel case, calling it "ConfigMap" instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

These paragraphs are clear. Since the reference section has changed, I suggest to clean up the link to the API reference.
For example:

[API Reference](/docs/reference/kubernetes-api/)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍will fix


Don't split the API object name into separate words. For example, use
PodTemplateList, not Pod Template List.

Refer to API objects without saying "object," unless omitting "object"
leads to an awkward construction.
The following examples focus on capitalization. Review the related guidance on [Code Style](#code-style-inline-code) for more information on formatting API objects.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need to describe what to expect in the following table. How about removing line 56?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added that line to offer an hint as to why code style appeared in the table, so I think it has a purpose.

The two ContainerStateTerminated objects ... | The two ContainerStateTerminateds ...
The `Volume` object contains a `hostPath` field. | The volume object contains a hostPath field.
Every `ConfigMap` is part of a namespace. | Every configMap is part of a namespace.
For managing confidential data, consider using the Secret API. | For managing confidential data, consider using a secret.
Copy link
Contributor

Choose a reason for hiding this comment

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

👋 @geoffcline .

Does the Secret example represent this capitalization case:
When you are generally discussing an API object, use [sentence-style capitalization]

Personal opinion: I am not convinced the site needs to format API resources and definitions as code.
To be consistent, any example in this guide should format API objects with the recommended style.
Otherwise, if there are remaining questions about formatting, raise an issue. Lines 62-64 add new content and should follow the style guide. Lines 62 and 63 display an API resource, definition, and field name in code format.

Would this example fit with the current style and capitalization recommendations?

For managing confidential data, consider using a `Secret`. | For managing confidential data, consider using a secret.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for reviewing again.

Did you see my discussion on this issue with Qiming? #24645 (comment)

I don't think our style guide mandates code style yet, so I wanted to illustrate the different options.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 25, 2021
Do | Don't
:--| :-----
The pod has two containers. | The Pod has two containers.
The HorizontalPodAutoscaler is responsible for ... | The HorizontalPodAutoscaler object is responsible for ...
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @geoffcline . I'd like to move this PR forward or close it.
I think there is agreement with the guidance in the section on capitalization and API objects.
What do you think about limiting the examples? For example:

The HorizontalPodAutoscaler resource is responsible for ... | The Horizontal pod autoscaler is responsible for ...
A PodList object is a list of pods. | A Pod List object is a list of pods.
The Volume object contains a `hostPath` field. | The volume object contains a hostPath field.
Every ConfigMap object is part of a namespace. | Every configMap object is part of a namespace.
For managing confidential data, consider using the Secret API. | For managing confidential data, consider using the secret API.

Copy link
Contributor

Choose a reason for hiding this comment

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

One thing to watch out for: you can find a volume mentioned in Kubernetes in several places, but AIUI not as a resource in the HTTP API. I'm wary in case someone reads the style guide and ends up confused about how storage works.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK @sftim. Can you provide a concrete example?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could remove the "Volume" example. The examples I provided are "tests" of the recommendations.

Copy link
Contributor

Choose a reason for hiding this comment

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

storage volume, volume plugin, a volume or logical drive, volume driver

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree on moving this forward or closing it.

I'm thinking I'll bring this up again at the next sig docs meeting to ask for synchronous feedback. I'll make a powerpoint slide with the new examples.

Regarding the volume example, I attempted to literally copy it from the API reference.

See, for example,

@kbhawkey
Copy link
Contributor

kbhawkey commented Feb 3, 2021

@geoffcline , These changes seem good enough to merge (add recommendations/flexibility about using resource/object with
a resource type).
You could rebase the commits into a single commit.
Are there other concerns/questions about code formatting?
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 3, 2021
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: f7a698ea24a74152b411d3891bb59134be0446fd

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 5, 2021
@geoffcline
Copy link
Contributor Author

Rebase completed ✔

Copy link
Member

@irvifa irvifa left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 7, 2021
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 21f6a0fbbed147feaba9bfb0c2a2041f76b5bb6a

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: irvifa

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 7, 2021
@k8s-ci-robot k8s-ci-robot merged commit 64b63c0 into kubernetes:master Feb 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants