Skip to content
This repository was archived by the owner on Apr 17, 2025. It is now read-only.

docs: improve grammar and simplify HRQ examples #315

Merged
merged 1 commit into from
Oct 26, 2023

Conversation

agilgur5
Copy link
Contributor

@agilgur5 agilgur5 commented Jul 4, 2023

Summary

Was reading through the docs and these seemed the most verbose

Details

  • use same heading in both Quickstart and Concepts, including HRQ abbreviation
    • use abbreviation to shorten the HRQ docs themselves as well

Grammar

  • fix run-on sentences by adding punctuation
    • "by default, to use it" -> "by default. To use it"
    • and a few others
  • add missing commas
    • "Then,", "finally,", etc
  • add Oxford commas
    • ", but", ", and", ", or", etc

Simplifications

  • simplify several sentences, per k8s style guide

    • "won't be over the amount of resources that is configured in HierarchicalResourceQuota" -> "cannot surpass the HRQ"
      • "can't go over what is configured in the HRQ" -> "can't surpass the HRQ"
    • "And put the resources HRQ in" -> "An HRQ in"
      • "This will restrict whole" -> "restricts" (also uses present tense now, per style guide)
    • "the amount of resources that" -> "the resources"
      • "resource usage" -> "resources"
    • "automatically creates" -> "creates"
  • in a similar vein, remove or replace ambiguous language

    • "equally shared" -> "shared" -- they can be shared however, not necessarily "equally"
    • "which is very efficient" -> "" -- "efficient" could be interpreted in a performance sense and is unnecesary
    • "lower level", "level" -> "child" / "sub" -- use HNC's existing terminology instead of "lower level", which has multiple disambiguations
      • Similarly use HNC's existing terminology for:
        • "individual", "members" -> "namespace", "subnamespaces"
        • "above", "overall" -> "parent"
    • "wins" -> "applies" -- replace idiom, per style guide
    • also remove the word "simply", per style guide
  • clarify a few sentences with a single word

    • "within", "Instead", "still", "directly"
  • consolidate the two paragraphs on command-line usage into one

    • looks like they were added at different times, in Documentation for #295 #298
    • they largely duplicated each other with slightly different wording
    • creating an HRQ via a CR YAML is the definition of how CRs work, which are already used across HNC, so not necessary to re-explain

Review Notes

There are potentially more changes that could be made according to the style guide, but I primarily focused changes on simplifications, clarifications, and grammar

Misc

This is a great example! It gives very practical, real-world use-cases which is very helpful for context and understanding how one might use HNC!

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 4, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @agilgur5. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 4, 2023
@k8s-ci-robot k8s-ci-robot requested review from rjbez17 and srampal July 4, 2023 19:39
@agilgur5 agilgur5 force-pushed the docs-simplify-hrq branch from 04d6342 to 7ab1912 Compare July 4, 2023 19:48
@agilgur5 agilgur5 force-pushed the docs-simplify-hrq branch from 7ab1912 to da5cd04 Compare July 4, 2023 19:57
Copy link
Contributor

@adrianludwin adrianludwin left a comment

Choose a reason for hiding this comment

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

Thanks for this change!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 15, 2023
@adrianludwin
Copy link
Contributor

Hi @agilgur5 , sorry for the long delay, but could you please rebase this change? I think the tests will pass after that's done. Thanks!

Was reading through the docs and these seemed the most verbose
- it seems to be a very recent feature as well, from 3aef487

- use same heading in both Quickstart and Concepts, including HRQ abbreviation
  - use abbreviation to shorten the HRQ docs themselves as well

- fix run-on sentences by adding punctuation
  - "by default, to use it" -> "by default. To use it"
- add missing commas
  - "Then,", "finally,", etc
- add Oxford commas
  - ", but", ", and", ", or", etc

- simplify several sentences, per [k8s style guide](https://kubernetes.io/docs/contribute/style/style-guide/#use-simple-and-direct-language)
  - "won't be over the amount of resources that is configured in `HierarchicalResourceQuota`" -> "cannot surpass the HRQ"
    - "can't go over what is configured in the HRQ" -> "can't surpass the HRQ"
  - "And put the resources HRQ in" -> "An HRQ in"
    - "This will restrict whole" -> "restricts" (also uses present tense now, per style guide)
  - "the amount of resources that" -> "the resources"
    "resource usage" -> "resources"
  - "automatically creates" -> "creates"
- in a similar vein, remove or replace ambiguous language
  - "equally shared" -> "shared" -- they can be shared however, not necessarily "equally"
  - "which is very efficient" -> "" -- "efficient" could be interpreted in a performance sense and is unnecesary
  - "lower level", "level" -> "child" / "sub" -- use HNC's existing terminology instead of "lower level", which has multiple disambiguations
    - Similarly use HNC's existing terminology for:
      - "individual", "members" -> "namespace", "subnamespaces"
      - "above", "overall" -> "parent"
  - "wins" -> "applies" -- replace idiom, per [style guide](https://kubernetes.io/docs/contribute/style/style-guide/#avoid-jargon-and-idioms)

- clarify a few sentences with a single word
  - "within", "Instead", "still", "directly"

- consolidate the two paragraphs on command-line usage into one
  - looks like they were added at different times, in 564b4a9
  - they largely duplicated each other with slighlty different wording
  - creating an HRQ via a CR YAML is the definition of how CRs work, which are already used across HNC, so not necessary to re-explain
    - also remove the word "simply", per [style guide](https://kubernetes.io/docs/contribute/style/style-guide/#avoid-words-that-assume-a-specific-level-of-understanding)

There are potentially more changes that could be made according to the style guide, but I primarily limited changes to simplifications, clarifications, and grammar
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 18, 2023
@agilgur5
Copy link
Contributor Author

@adrianludwin rebased. It needs to be re-approved now

@agilgur5 agilgur5 requested a review from adrianludwin October 4, 2023 21:45
@rjbez17
Copy link
Contributor

rjbez17 commented Oct 26, 2023

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrianludwin, agilgur5, rjbez17

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 [adrianludwin,rjbez17]

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

@rjbez17
Copy link
Contributor

rjbez17 commented Oct 26, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 26, 2023
@k8s-ci-robot k8s-ci-robot merged commit a68b2f8 into kubernetes-retired:master Oct 26, 2023
@agilgur5 agilgur5 deleted the docs-simplify-hrq branch October 26, 2023 21:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants