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

docs: add guidelines for contributing to Cilium's documentation #16738

Merged
merged 2 commits into from Jul 2, 2021

Conversation

qmonnet
Copy link
Member

@qmonnet qmonnet commented Jul 1, 2021

This PRs a set of recommendations on style and formatting for contributing to Cilium's documentation. The current guidelines are mostly about RST formatting for now, but can be extended with whatever directions may help produce a better documentation in the future.

Some of these guidelines are simply suggestions: they may not have been the topic of any particular discussion before, and feedback is welcome. I spent some time looking at the RST specification, but I don't claim to have the right point-of-view on every item.

One follow-up item to add would be a recommendation on the structure, in particular regarding nesting (or not nesting) RST fragments. See this discussion with suggestions from @nbusseneau for reference, but I'm not sure a consensus was reached at the time.

[First commit also relates to the documentation, but focuses on syntax highlighting and cancels Sphinx's default attempt to highlight snippets based on Python3's syntax.]

Sphinx attempts to apply syntax highlighting on literal blocks (the
blocks marked with "code-block" directives with no language provided,
"parsed-literal" directives, or literal block markers ("::"). The
documentation [1] explains that it defaults on Python:

> The default language to highlight source code in. The default is
> 'default'. It is similar to 'python3'; it is mostly a superset of
> 'python' but it fallbacks to 'none' without warning if failed.
> 'python3' and other languages will emit warning if failed.

We barely use any Python in the repository. When no language is
provided, this is most of the time because the snippet is just some
unstructured code that should be displayed verbatim, or, in the case of
parsed literals, commands to type in a terminal. Let's overwrite the
default and prevent the default highlighting based on Python3's syntax.

[1] https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-highlight_language

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@qmonnet qmonnet added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/misc This PR makes changes that have no direct user impact. labels Jul 1, 2021
@qmonnet qmonnet requested review from a team as code owners July 1, 2021 15:43
@nbusseneau
Copy link
Member

nbusseneau commented Jul 1, 2021

See this discussion with suggestions from @nbusseneau for reference, but I'm not sure a consensus was reached at the time.

We had a short discussion on Slack, and the conclusion was that it was better to fix structural issues when they arise, since they are quite rare. Until we happen to have the same kind of breakage all the time, no need to have guidelines for that :)

Copy link
Member

@nbusseneau nbusseneau left a comment

Choose a reason for hiding this comment

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

Read through, LGTM. I was thinking we should maybe have an additional section regarding formatting, notably for indentation and whitespace around various common blocks. What do you think?

Copy link
Member

@aditighag aditighag left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@qmonnet
Copy link
Member Author

qmonnet commented Jul 1, 2021

Read through, LGTM. I was thinking we should maybe have an additional section regarding formatting, notably for indentation and whitespace around various common blocks. What do you think?

Not a bad idea, but I don't think there's a consensus on that - I think that for code blocks for example we have a mix of 2, 3, 4, 8 spaces in different files (or sometimes in a single one). And if we make it too strict, nobody will follow it. Do you have suggestions in particular?

One thing that comes to mind is that we prefer to wrap long lines, I forgot to mention that.

@nbusseneau
Copy link
Member

Do you have suggestions in particular?

No, I was mostly just confused when contributing about what rules I should follow. So far I've just rolled with using the same style as the one around what I was modifying.

Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

💯

Add a set of recommendations on style and formatting for contributing to
Cilium's documentation. The current guidelines are mostly about RST
formatting for now, but can be extended with whatever directions may
help produce a better documentation in the future.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@qmonnet
Copy link
Member Author

qmonnet commented Jul 1, 2021

I added a (hopefully consensual) note on line wrapping:

Body
----

Wrap the lines for long sentences or paragraphs. There is no fixed convention
on the length of lines, but targeting a width of about 80 characters should be
safe in most circumstances.

Based on the reviews and the Documentation action passing, I'll mark as ready-to-merge. I'm happy to add a note on indent and spacing as a follow-up if we settle on something.

@qmonnet qmonnet added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 1, 2021
@pchaigno pchaigno merged commit 60c5759 into cilium:master Jul 2, 2021
@qmonnet qmonnet deleted the pr/doc-style-guidelines branch July 2, 2021 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants