-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
docutils
: Use ClassVar
for Directive
class variables
#11550
Conversation
@danieleades Does this change replicate any of your recent docutils PRs? (just making sure I'm not duplicating what you already wrote) |
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This type change would definitely have downstream ramifications for sphinx. @sphinx-doc: As downstream consumers of docutils, any opinions or disagreements on This also connects to Sphinx's ignored ruff rule |
it doesn't. In fact it looks like i introduced the issue here - #11523. Rather than adding |
In that case, this change would be beneficial. And to paraphrase, would eliminate the need for subclasses of |
it should remove the need for the subclass stubs to include those members at all, since they are inherited from the base class |
This comment has been minimized.
This comment has been minimized.
63cf19b
to
93e41d9
Compare
e1e9345
to
b3af292
Compare
This comment has been minimized.
This comment has been minimized.
634cf7a
to
b3af292
Compare
This comment has been minimized.
This comment has been minimized.
I don't think it stop this PR from getting merged, but you might want to see if the maintainers of I see pygments/pygments#1189 was closed because the library still supported python 2.7, but this has now been dropped. Always better to have the type annotations inline if you possibly can, although you'd probably also want to add a |
This comment has been minimized.
This comment has been minimized.
that's a lot of downstream errors. Might be best to get one of the grand wizards of typeshed to comment on this (@JelleZijlstra, @srittau ?) but i suspect the subclasses in This issue makes me think these errors should be suppressed already, but here we are. For what it's worth, |
It's a separate topic, in fact I would recommend you create an issue to rekindle interest in them adding annotations (perhaps by starting with typeshed's). Anecdote: I had to add custom types for pygments recently in another project. Perhaps a lot of users could be impacted by pygments needing types. And of course, better to have them straight from the tap. |
@danieleades My earlier comment: #11550 (comment) A minor nit: I don't see them as errors, but downstream updates that'd need to be updated on sphinx's side after this change. @tk0miya, @AA-Turner (via @sphinx-doc):
|
src/doctest_docutils.py:145: error: Cannot override instance variable (previously declared on base class "Directive") with class variable [misc] src/doctest_docutils.py:151: error: Cannot override instance variable (previously declared on base class "Directive") with class variable [misc] src/doctest_docutils.py:157: error: Cannot override instance variable (previously declared on base class "Directive") with class variable [misc] Found 3 errors in 1 file (checked 11 source files) See also: python/typeshed#11550
i agree |
src/doctest_docutils.py:145: error: Cannot override instance variable (previously declared on base class "Directive") with class variable [misc] src/doctest_docutils.py:151: error: Cannot override instance variable (previously declared on base class "Directive") with class variable [misc] src/doctest_docutils.py:157: error: Cannot override instance variable (previously declared on base class "Directive") with class variable [misc] Found 3 errors in 1 file (checked 11 source files) See also: python/typeshed#11550
src/doctest_docutils.py:145: error: Cannot override instance variable (previously declared on base class "Directive") with class variable [misc] src/doctest_docutils.py:151: error: Cannot override instance variable (previously declared on base class "Directive") with class variable [misc] src/doctest_docutils.py:157: error: Cannot override instance variable (previously declared on base class "Directive") with class variable [misc] Found 3 errors in 1 file (checked 11 source files) See also: python/typeshed#11550
src/doctest_docutils.py:145: error: Cannot override instance variable (previously declared on base class "Directive") with class variable [misc] src/doctest_docutils.py:151: error: Cannot override instance variable (previously declared on base class "Directive") with class variable [misc] src/doctest_docutils.py:157: error: Cannot override instance variable (previously declared on base class "Directive") with class variable [misc] Found 3 errors in 1 file (checked 11 source files) See also: python/typeshed#11550
b3af292
to
2997283
Compare
This comment has been minimized.
This comment has been minimized.
Hopefully this gets merged soon. This fixes some downstream errors in my own repos that i inadvertently introduced in #11523 |
These are intended to be set as class variables, in subclasses of Directive, rather than instance variables. See also: - https://docutils.sourceforge.io/docs/howto/rst-directives.html#the-directive-class - https://docutils.sourceforge.io/docs/howto/rst-directives.html#admonitions
stubs/Pygments/pygments/sphinxext.pyi:11-15: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
`Directives` adding these as `ClassVar` eliminates the need for its subclasses to. Co-authored-by: danieleades <[email protected]>
Co-authored-by: danieleades <[email protected]>
2997283
to
c8f0c22
Compare
@danieleades @JelleZijlstra @srittau Rebased at 5b1fd12 |
Diff from mypy_primer, showing the effect of this PR on open source code: sphinx (https://github.com/sphinx-doc/sphinx)
+ sphinx/directives/__init__.py: note: In class "ObjectDescription":
+ sphinx/directives/__init__.py:58:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/directives/__init__.py: note: At top level:
+ sphinx/directives/__init__.py: note: In class "DefaultDomain":
+ sphinx/directives/__init__.py:345:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/std/__init__.py: note: In class "Target":
+ sphinx/domains/std/__init__.py:115:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/std/__init__.py: note: In class "Program":
+ sphinx/domains/std/__init__.py:245:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/std/__init__.py: note: In class "Glossary":
+ sphinx/domains/std/__init__.py:309:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/std/__init__.py: note: At top level:
+ sphinx/domains/std/__init__.py: note: In class "ProductionList":
+ sphinx/domains/std/__init__.py:456:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/python/_object.py: note: In class "PyObject":
+ sphinx/domains/python/_object.py:147:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/python/__init__.py: note: In class "PyFunction":
+ sphinx/domains/python/__init__.py:70:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/python/__init__.py: note: In class "PyVariable":
+ sphinx/domains/python/__init__.py:125:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/python/__init__.py: note: In class "PyClasslike":
+ sphinx/domains/python/__init__.py:164:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/python/__init__.py: note: In class "PyMethod":
+ sphinx/domains/python/__init__.py:192:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/python/__init__.py: note: In class "PyClassMethod":
+ sphinx/domains/python/__init__.py:246:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/python/__init__.py: note: In class "PyStaticMethod":
+ sphinx/domains/python/__init__.py:258:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/python/__init__.py: note: In class "PyAttribute":
+ sphinx/domains/python/__init__.py:286:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/python/__init__.py: note: In class "PyProperty":
+ sphinx/domains/python/__init__.py:331:5: error: Cannot override instance variable (previously declared on base class "PyObject") with class variable [misc]
+ sphinx/domains/python/__init__.py: note: In class "PyModule":
+ sphinx/domains/python/__init__.py:388:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/python/__init__.py: note: In class "PyCurrentModule":
+ sphinx/domains/python/__init__.py:447:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/ext/todo.py: note: In class "Todo":
+ sphinx/ext/todo.py:56:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/ext/todo.py: note: In class "TodoList":
+ sphinx/ext/todo.py:115:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/rst.py: note: In class "ReSTMarkup":
+ sphinx/domains/rst.py:39:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/rst.py: note: In class "ReSTDirectiveOption":
+ sphinx/domains/rst.py:145:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/javascript.py: note: In class "JSObject":
+ sphinx/domains/javascript.py:49:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/javascript.py: note: In class "JSModule":
+ sphinx/domains/javascript.py:301:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/index.py: note: In class "IndexDirective":
+ sphinx/domains/index.py:71:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/changeset.py: note: In class "VersionChange":
+ sphinx/domains/changeset.py:55:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/cpp/__init__.py: note: In class "CPPObject":
+ sphinx/domains/cpp/__init__.py:68:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/cpp/__init__.py: note: In class "CPPNamespaceObject":
+ sphinx/domains/cpp/__init__.py:387:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/cpp/__init__.py: note: In class "CPPNamespacePushObject":
+ sphinx/domains/cpp/__init__.py:418:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/cpp/__init__.py: note: In class "CPPNamespacePopObject":
+ sphinx/domains/cpp/__init__.py:450:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/cpp/__init__.py: note: In class "CPPAliasObject":
+ sphinx/domains/cpp/__init__.py:627:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/c/__init__.py: note: In class "CObject":
+ sphinx/domains/c/__init__.py:59:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/c/__init__.py: note: In class "CNamespaceObject":
+ sphinx/domains/c/__init__.py:300:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/c/__init__.py: note: In class "CNamespacePushObject":
+ sphinx/domains/c/__init__.py:330:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/c/__init__.py: note: In class "CNamespacePopObject":
+ sphinx/domains/c/__init__.py:361:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/domains/c/__init__.py: note: In class "CAliasObject":
+ sphinx/domains/c/__init__.py:520:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/ext/ifconfig.py: note: In class "IfConfig":
+ sphinx/ext/ifconfig.py:44:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/ext/doctest.py: note: In class "TestsetupDirective":
+ sphinx/ext/doctest.py:146:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/ext/doctest.py: note: In class "TestcleanupDirective":
+ sphinx/ext/doctest.py:152:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/ext/doctest.py: note: In class "DoctestDirective":
+ sphinx/ext/doctest.py:158:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/ext/doctest.py: note: In class "TestcodeDirective":
+ sphinx/ext/doctest.py:169:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/ext/doctest.py: note: In class "TestoutputDirective":
+ sphinx/ext/doctest.py:179:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/ext/autosummary/__init__.py: note: In class "Autosummary":
+ sphinx/ext/autosummary/__init__.py:221:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/directives/patches.py: note: In class "Code":
+ sphinx/directives/patches.py:85:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/directives/patches.py: note: In class "MathDirective":
+ sphinx/directives/patches.py:130:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/directives/other.py: note: In class "Author":
+ sphinx/directives/other.py:182:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/directives/other.py: note: In class "TabularColumns":
+ sphinx/directives/other.py:224:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/directives/other.py: note: In class "Centered":
+ sphinx/directives/other.py:242:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/directives/other.py: note: In class "Acks":
+ sphinx/directives/other.py:265:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/directives/other.py: note: In class "HList":
+ sphinx/directives/other.py:288:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/directives/other.py: note: In class "Only":
+ sphinx/directives/other.py:326:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/directives/other.py: note: At top level:
+ sphinx/directives/code.py: note: In class "Highlight":
+ sphinx/directives/code.py:38:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/directives/code.py: note: In class "CodeBlock":
+ sphinx/directives/code.py:105:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/directives/code.py: note: In class "LiteralInclude":
+ sphinx/directives/code.py:396:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/ext/graphviz.py: note: In class "Graphviz":
+ sphinx/ext/graphviz.py:120:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/ext/graphviz.py: note: In class "GraphvizSimple":
+ sphinx/ext/graphviz.py:189:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ sphinx/ext/inheritance_diagram.py: note: In class "InheritanceDiagram":
+ sphinx/ext/inheritance_diagram.py:352:5: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
alectryon (https://github.com/cpitclaudel/alectryon)
+ alectryon/docutils.py:802: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
+ alectryon/docutils.py:1120: error: Cannot override class variable (previously declared on base class "Directive") with instance variable [misc]
|
That's some scare mypy-primer output, but it sounds like it's expected. |
FWIW this has caused type errors in my project. I subclass class SubstitutionCodeBlock(CodeBlock):
"""
Similar to CodeBlock but replaces placeholders with variables.
"""
option_spec = CodeBlock.option_spec
option_spec["substitutions"] = directives.flag and I get the following error:
|
this is an issue in You may wish to raise a PR or issue in the Sphinx repo |
This issue led to a failure downstream in jaraco/jaraco.packaging#14. |
Declare ``SidebarLinksDirective.has_content`` as a class variable to align with changes made in python/typeshed#11550. Closes #14.
The solution is to annotate the class variables in the subclass using |
And indeed, that's what I've done. It was a little annoying that this enhanced specification broke the build. I'm also a little worried that it's over constraining. After all, a subclass could customize the instance attribute and that still would have the intended effect but wouldn't pass static analysis. But I guess that's not my problem, so I'll leave it for someone else to adjudicate. |
These variables of
Directive
are expected and widely accepted in practice as class variables, useClassVar
for these.Brief example
Real world case
Without this, subclasses will error like this in mypy 1.9.0:
Side note:
ruff
'sRUF012
Without debating the merits of the lint rule itself, I've anecdotally had multiple projects using ruff and
Directive
that have encounteredmutable-class-default (RUF012)
multiple times.Further documentation
Docstring for `Directive`
via https://docutils.sourceforge.io/docs/howto/rst-directives.html#the-directive-class (accessed 2024-03-09).
Example of a `Directive` subclass from docutils documentation
via https://docutils.sourceforge.io/docs/howto/rst-directives.html#admonitions (accessed 2024-03-09).
Links