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

Documentation consistency improvements #59093

Merged
merged 13 commits into from Feb 19, 2021
Merged

Conversation

eliasp
Copy link
Contributor

@eliasp eliasp commented Dec 5, 2020

What does this PR do?

It cleans up many smaller inconsistencies in the docs, such as:

  • lack of rST verbatim usage (like code in Markdown)
  • lack of rST references (e.g. when referencing another module/function)
  • proper usage of field lists for function parameters/return data, etc.
  • inconsistent module names in module listings
  • ...

Each logical change is a separate commit.

What issues does this PR fix or reference?

None

Merge requirements satisfied?

Commits signed with GPG?

Yes

@eliasp eliasp force-pushed the doc-consistency branch 2 times, most recently from 8137daf to 245e86e Compare December 5, 2020 17:55
@eliasp eliasp marked this pull request as ready for review December 14, 2020 14:09
@eliasp eliasp requested a review from a team as a code owner December 14, 2020 14:09
@eliasp eliasp requested review from Ch3LL and removed request for a team December 14, 2020 14:09
@eliasp
Copy link
Contributor Author

eliasp commented Dec 14, 2020

Don't have time right now to add further improvements, so I'm switching from WIP to "Ready for Review" and also just rebased it on latest master.

Ch3LL
Ch3LL previously approved these changes Dec 15, 2020
@Ch3LL Ch3LL added the Aluminium Release Post Mg and Pre Si label Dec 15, 2020
- use "verbatim" for variables etc.
- consistent upper-casing for acronyms (e.g. CLI)
- use rST for return data and return types
- use :mod: to link to related modules/functions
- correct code type for Jinja snippet
Until now, the listings of all kinds of modules looked quite
inconsistent like this:
- salt.states.smtp
- salt.states.snapper module
- salt.states.solrcloud module
- salt.states.splunk

The `module` suffix is redundant anyways, so let's remove it and do some
further cleanup (adjusting overline/underline usage, underline length)
using the following bash script:

```bash

sed -i 's|\s\+module$||g' doc/ref/*/all/*.*.rst

for rst in doc/ref/*/all/*.*.rst;
do
  # enforce consistent overline/underline usage, remove all overlines
  if [[ $(head -n1 "${rst}") =~ ^=+$ ]];
  then
      sed -i -e 1d "${rst}"
  fi

  # adjust the length of the underline to the length of the module name
  module_name=$(head -n1 "${rst}")
  underline=$(for i in $(seq ${#module_name}); do echo -n "="; done)
  sed -i '2s|^=\+$|'"${underline}"'|g' "${rst}"
done
```
The bug was fixed 7 years ago, the distribution release in question was
EOL'd 1,5 years ago.
- use "verbatim" for variables etc.
- use :mod: to link to related modules/functions
- fix indentation, causing a parameter doc to be displayed as quote
- use PGP instead of GPG where appropriate (PGP is the standard, GPG/GnuPG the implementation)
- minor grammar/wording improvements
@eliasp
Copy link
Contributor Author

eliasp commented Feb 15, 2021

Rebased it on latest saltstack:master to fix the merge conflicts.

@eliasp eliasp requested a review from Ch3LL February 15, 2021 13:32
Ch3LL
Ch3LL previously approved these changes Feb 17, 2021
Copy link
Contributor

@ScriptAutomate ScriptAutomate left a comment

Choose a reason for hiding this comment

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

My only feedback is with the changes to salt/states/pkgrepo.py when it comes to GPG being changed to PGP. Is there a good justification for why this needs to be modified in this states module? Isn't GPG an open source standard implementation of PGP, and that most of these should still be saying GPG? It's difficult for me to discern whether any reference to a "GPG key" is an accidental online phenomenon of people incorrectly using it interchangeably with "PGP key" or whether the true definition is always a PGP key that is being mistaken. It seems that since it is a non-proprietary software implementation of the PGP standard, that GPG may make sense to continue using?

That, or perhaps we can use PGP/GPG if they are meant to be interchangeable, allowing for both to appear when searching docs?

salt/states/pkgrepo.py Outdated Show resolved Hide resolved
salt/states/pkgrepo.py Outdated Show resolved Hide resolved
salt/states/pkgrepo.py Outdated Show resolved Hide resolved
salt/states/pkgrepo.py Outdated Show resolved Hide resolved
salt/states/pkgrepo.py Outdated Show resolved Hide resolved
salt/states/pkgrepo.py Outdated Show resolved Hide resolved
salt/states/pkgrepo.py Outdated Show resolved Hide resolved
@ScriptAutomate
Copy link
Contributor

ScriptAutomate commented Feb 17, 2021

Side note for those doomed to try reviewing a major amount of diffs in GitHub for a single PR similar to this PR in nature:

  • Scroll down the entire list of changes so that GitHub loads all of the collapsible boxes associated with all of the changes
  • Open up the inspector/console in the browser devtools. This is probably accessed easily with ctrl+i. We want to be in the Console, which in Firefox is ctrl+shift+k.
  • Type in the following to expand all diffs, in order to avoid needing to click on hundreds of Load diff buttons:
Array.from(document.getElementsByClassName('load-diff-button')).map(button => button.click())

Wish GitHub had a Load all diffs button. This took a lot of time to eyeball.

Source GitHub conversation where I found the snippet:

I've added this, along with some other information, to a gist I'm using for collecting helpful snippets like this:

@eliasp
Copy link
Contributor Author

eliasp commented Feb 17, 2021

That, or perhaps we can use PGP/GPG if they are meant to be interchangeable, allowing for both to appear when searching docs?

  • correctness: PGP
  • searchability: PGP/GPG
  • compromise: PGP/GnuPG

I'm undecided :)

Co-authored-by: Derek Ardolf <ScriptAutomate@users.noreply.github.com>
@ScriptAutomate
Copy link
Contributor

ScriptAutomate commented Feb 18, 2021

That, or perhaps we can use PGP/GPG if they are meant to be interchangeable, allowing for both to appear when searching docs?

* correctness: `PGP`

* searchability: `PGP/GPG`

* compromise: `PGP/GnuPG`

I'm undecided :)

I'm thinking of leaving with gpg for now, due to much of configs and codebase making references to gpg keys:

egrep -rn "gpgkey|gpg_key"

Due to that, and with other docsets such as the GitHub, GitLab, and git docs as examples:

If you have a GPG private key setup, you can now use it to sign new tags. All you have to do...

GitLab seems to use GPG key but also provides the following message about their docs:

The term GPG is used for all OpenPGP/PGP/GPG related material and implementations.

This seems important, because keys will be having -----BEGIN PGP SIGNATURE----- within them, too, to your point on technical correctness.

@eliasp I'm going to go ahead and apply the commit suggestions I've made for GPG in the docs descriptions. I'm going to also create an issue to use GitLab's approach so that we can have an .. include:: of a reusable .. note:: admonition about the use of GPG in reference to OpenPGP/PGP/GPG.

UPDATE: Issue created: #59539

@Ch3LL Ch3LL merged commit 12f9b98 into saltstack:master Feb 19, 2021
@eliasp eliasp deleted the doc-consistency branch February 19, 2021 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Aluminium Release Post Mg and Pre Si
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants