-
-
Notifications
You must be signed in to change notification settings - Fork 32.8k
Fixed #36485 -- Added linting tool for documentation files. #19549
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some initial comments for first commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @smithdc1 to invest time in this work. I think we are in the right direction! 🌟
I was wondering if you consider making this an extension: we would still need some customization/wrangling, but we would avoid all the new Makefile rule/tox changes/etc etc.
I gave this a try and have this MVP (still needs the customizations). In an ideal scenario, this would replace commits 1 and 6:
commit 48075d9be469154fb26abde37b955a39c2785b4a (HEAD -> pr/19549)
Author: Natalia <124304+nessita@users.noreply.github.com>
Date: Thu Jun 12 14:36:25 2025 -0300
Alternative to explicit `lint` Makefile rule via Sphinx Extensions.
diff --git a/docs/_ext/djangolint.py b/docs/_ext/djangolint.py
new file mode 100644
index 0000000000..4009dc5524
--- /dev/null
+++ b/docs/_ext/djangolint.py
@@ -0,0 +1,28 @@
+from sphinx.util import logging
+from sphinxlint.checkers import all_checkers
+from sphinxlint.sphinxlint import check_text
+
+logger = logging.getLogger(__name__)
+
+
+def lint_file(app, docname, source):
+ path = app.env.doc2path(docname)
+ content = source[0]
+
+ checkers = list(all_checkers.values())
+ for checker in checkers:
+ if ".rst" in checker.suffixes:
+ checker.suffixes = (*checker.suffixes, ".txt")
+
+ errors = check_text(path, content, checkers)
+ for error in errors:
+ logger.error(error)
+
+
+def setup(app):
+ app.connect("source-read", lint_file)
+ return {
+ "version": "1.0",
+ "parallel_read_safe": True,
+ "parallel_write_safe": True,
+ }
diff --git a/docs/conf.py b/docs/conf.py
index be79b9133c..1836eed3df 100644
--- a/docs/conf.py
+++ b/docs/conf.py
@@ -44,6 +44,7 @@ needs_sphinx = "4.5.0"
# coming with Sphinx (named 'sphinx.ext.*') or your custom ones.
extensions = [
"djangodocs",
+ "djangolint",
"sphinx.ext.extlinks",
"sphinx.ext.intersphinx",
"sphinx.ext.autosectionlabel",
For long lines that are links, there is an opened PR against the lint project: sphinx-contrib/sphinx-lint#132 |
I'm not sure. This would be a different approach taken with both
|
Thank you for this reasoning, these are all very valid points. My rationale was along these lines:
Re: |
Thank you Natalia. One other thing I thought about is having it available as a pre-commit hook. It seems that a separate repository would be required though and that feels a bit much. I think I'd rather try and discuss these changes with the sphinx-lint team and see if I can contribute the changes back upstream. This is a longer term goal though and shouldn't stop this work here. Now that we've had a good discussion on the pro/cons of some different approaches I'm happy to help implement this change in the way that you recommend. |
My recommendation would be to push forward the approach that you think it's going to be less work for you, while providing useful feedback to the contributors. I sense that you may prefer the separated, non extension, lint rule and I'm happy if we go that path (as you said, we have covered on pro/cons and there is no clear "winner"). Perhaps a good next step for a small, self contained commit would be to add a new Makefile rule "check" that would do black, lint and spelling at once? |
^ Conflicts: ^ docs/Makefile
applications aren't ``washingtonpost.com`` or ``slashdot.org``; they're small- | ||
to medium-sized sites with so-so traffic. But for medium- to high-traffic | ||
applications aren't ``washingtonpost.com`` or ``slashdot.org``; they're | ||
small-to medium-sized sites with so-so traffic. But for medium- to high-traffic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noticed a small typo in the commit msg: hypen -> hyphen, cheers! 👋
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch 🎯
Congrats on the role of Django Fellow! 🥳 🥳 🥳
Checking documentation in proposed patches for adherence to the 'style guide' takes time for the Fellows to review and update if required. The line length guide of 80 characters is a stand out here.
There's a tool called sphinx-lint, which err, lints sphix files. This tool's project repo seems to be fairly active and so seems well supported.
Unfortunately, it doesn't work out of the box for us as Django uses
.txt
extensions and the line length check needs some adjustments to be suitable for Django's docs.This series of proposed patches does the following:
It will need some polishing, and I'm happy to do that, if there's support for this change.