Skip to content

[WIP] docs: Add rstcheck for generarated rst#15786

Closed
phlax wants to merge 3 commits intoenvoyproxy:mainfrom
phlax:docs-add-rstcheck
Closed

[WIP] docs: Add rstcheck for generarated rst#15786
phlax wants to merge 3 commits intoenvoyproxy:mainfrom
phlax:docs-add-rstcheck

Conversation

@phlax
Copy link
Member

@phlax phlax commented Mar 31, 2021

Commit Message: docs: Add rstcheck for generarated rst
Additional Description:

this is a follow on from #15766 to use rstcheck

it will take quite a lot more cleaning up so i was hoping it wouldnt block #15766

Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

@repokitteh-read-only repokitteh-read-only bot added api deps Approval required for changes to Envoy's external dependencies labels Mar 31, 2021
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #15786 was opened by phlax.

see: more, trace.

@phlax
Copy link
Member Author

phlax commented Mar 31, 2021

annoying rstcheck doesnt pick up the original error of ref: so i think it will be necessary to include the grep

@phlax phlax force-pushed the docs-add-rstcheck branch from d71a4f3 to 6f06328 Compare March 31, 2021 15:21
docs/build.sh Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

handle this better

@phlax phlax force-pushed the docs-add-rstcheck branch from c8f711f to 3bdfbeb Compare March 31, 2021 21:46
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Nice; looks good directionally, let me know when ready for a full review pass.

phlax added 2 commits April 19, 2021 08:32
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax phlax force-pushed the docs-add-rstcheck branch from 55a7697 to e9f8a59 Compare April 19, 2021 07:32
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax
Copy link
Member Author

phlax commented Apr 19, 2021

im a bit concerned that rstcheck seems to be unmaintained, and also has kinda obvious bugs that have been kicking around for ~6 years (rstcheck/rstcheck#76) without being addressed

all the same its kinda useful - so i think rather include it and add workarounds

@phlax
Copy link
Member Author

phlax commented Apr 19, 2021

surveying other linting tools for rst (again) they are seemingly similar - ie barely maintained if at all

the nature of rst is that it is complex (ie embeddable code) - not sure what is best here.


def parallel(self, cmd, args, handle):
errors = 0
pool = multiprocessing.Pool(multiprocessing.cpu_count())
Copy link
Member

Choose a reason for hiding this comment

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

Use a context manager with?

# things we dont want to see in generated docs
# TODO(phlax): move to .rstcheck.cfg when available
RSTCHECK_GREP_FAIL = (" ref:", "\\[\\#")
RSTCHECK_CONFIG=".rstcheck.cfg"
Copy link
Member

Choose a reason for hiding this comment

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

Nit: spacing (why doesn't our linter cover this?)

Copy link
Member Author

Choose a reason for hiding this comment

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

i think it does - flake8 might not because we dont enable most pep8 checks yet, but yapf isnt happy with it

@contextmanager
def sphinx_enabled():
"""Register Sphinx directives and roles."""
srcdir = tempfile.mkdtemp()
Copy link
Member

Choose a reason for hiding this comment

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

Why not with tempfile.TemporaryDirectory()?


def _rstcheck_handle(self, results: list) -> None:
"""Handle multiprocessed error results of rstchecks"""
for (filename, _errors) in results:
Copy link
Member

Choose a reason for hiding this comment

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

Nit: parens not needed here and below

@phlax phlax mentioned this pull request Apr 29, 2021
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label May 20, 2021
@phlax phlax removed the stale stalebot believes this issue/PR has not been touched recently label May 20, 2021
@phlax phlax added the no stalebot Disables stalebot from closing an issue label May 20, 2021
@alyssawilk alyssawilk closed this Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api deps Approval required for changes to Envoy's external dependencies no stalebot Disables stalebot from closing an issue waiting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants