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

refactor: enable linting of docs/custom_conf.py #1330

Merged

Conversation

james-garner-canonical
Copy link
Contributor

This PR reverts the only edit to docs/conf.py, which was made in this commit, and enables linting of docs/custom_conf.py

The resulting linting errors have been resolved primarily via tool.ruff.lint.per-file-ignores, with one inline noqa (copyright variable shadows builtin), and the removal of a type annotation not in scope (Node). It didn't seem worth spending significant time to add docstrings and resolve the other errors at this point. In fact, I'm not entirely sure whether it's worth merging in these changes, as it sounds like we've yet to settle on how the docs code will be handled with respect to upstream going forward, but I figured I might as well make a PR for review anyway in case it's helpful.

It's not worth importing the type because this file is largely not type
hinted anyway.
Changes should be made in docs/custom_conf.py where possible.

Enable custom_rst_epilog in docs/custom_conf.py (set to an empty string)
in lieu of removing rst_epilog from docs/conf.py (this commit restores
rst_epilog to docs/conf.py).

Remove global ruff: noqa directive for docs/conf.py -- ruff linting for
this file is already disabled in pyproject.toml
Re-enable linting for docs/custom_conf.py (removing global noqa), but
selectively disable tests that fail, as much of this code is based on
sphinx-docs-starter-pack/custom_conf.py, which does not use the same
linting as this project.
Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Thanks -- I was a bit on the fence about just ignoring all these warnings, but I think each of them seems reasonable to me in the context of custom_conf.py.

@james-garner-canonical
Copy link
Contributor Author

I agree that it's not ideal to disable so many warnings. Hopefully at least having the other linting checks enabled for the file will be helpful in future.

@james-garner-canonical james-garner-canonical merged commit 1a55b0b into canonical:main Aug 23, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants