-
-
Notifications
You must be signed in to change notification settings - Fork 31.5k
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
gh-129667: Update annotation in documentation #129669
base: main
Are you sure you want to change the base?
Conversation
IIRC there was a decision to not litter the docs the trailing backslash. People find it confusing and it doesn't seem to help them in any way. |
@rhettinger sorry, I fixed it. but other changes are relevant, isn't it? |
@rhettinger, the Editorial Board's recent decision was that PEP-570 syntax should be used in documentation. This is now documented in the devguide; see python/devguide#1344. The Editorial Board has a standing delegation from the Steering Council on all documentation matters: https://github.com/python/steering-council/blob/main/process/standing-delegations.md#documentation-editorial-board |
So should I put |
Then this PR can go forward. It's a bummer though. Signatures with trailing backslashes are intrinsically less readable. People who are only occasional users of Python find it confusing (likely because no other language does this). This makes the docs less accessible to average users. A high school student shouldn't have to see |
This reverts commit ec2398f.
I think they use other sources of knowledge, and most likely the docs reader is familiar with python syntax. And also it would be nice to add "skip news" label |
My direct experience says otherwise. When people take an introductory python course, the positional-only/keword-only syntax is rarely covered. There is just too much else to talk about. Introductory books on Python typically defer discussion on this topic until all other core topics have been covered. Also, it seems like an anti-goal to reduce accessiblity and expect some user categories to search elsewhere for knowledge. AFAICT, we've always wanted to maximize accessibility for all users. |
At a recent Editorial Board meeting, we re-affirmed the principle of the reference documentation being precise. The function signatures should have slashes to indicate arguments that are positional-only. It can be a bit confusing to new learners, so we'll be experimenting with adjustments to the Sphinx rendering to help people understand the syntax. |
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.
I'm a bit conflicted with the fact that we're now showing the internal parameters in the signatures. Yes, it could be helpful indeed, but it also exposes implementation details. For functions, I think we could implicitly not include them because an external caller is not meant to use them (probably), however for methods, if the class can be inherited, then children classes may want to know the complete signature just to be sure that they do not violate the Liskov substitution principle.
However, as I said on some functions/methods, I prefer that we don't add new parameters in this PR but only add the positional-only markers. In separate PRs, each added parameter should either explicitly be marked as internal (in the docs) or we should not show it if callers or subclasses do not need to know about it.
@@ -147,7 +147,7 @@ And:: | |||
executor.submit(wait_on_future) | |||
|
|||
|
|||
.. class:: ThreadPoolExecutor(max_workers=None, thread_name_prefix='', initializer=None, initargs=()) | |||
.. class:: ThreadPoolExecutor(max_workers=None, thread_name_prefix='', initializer=None, initargs=(), **ctxkwargs) |
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.
Shouldn't ctxkwargs be documented as well?
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.
I added a description and found PR #124548 where this argument was added
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.
As it's 3.14+, this means that this should not be backported. So a separate PR is easier to handle as well. And we can make it part of gh-124694 instead.
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.
Can you first address the new comments and then check that all the remaining modifications are actually backportable to 3.12 as well? TiA
@@ -112,7 +112,7 @@ Interactive Interpreter Objects | |||
with it. | |||
|
|||
|
|||
.. method:: InteractiveInterpreter.showsyntaxerror(filename=None) | |||
.. method:: InteractiveInterpreter.showsyntaxerror(filename=None, **kwargs) |
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.
I don't really know what kwargs means here so I would appreciate that it's documented in a separate PR as well.
@@ -194,6 +194,10 @@ And:: | |||
Default value of *max_workers* is changed to | |||
``min(32, (os.process_cpu_count() or 1) + 4)``. | |||
|
|||
.. versionchanged:: next |
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.
It's not "next" as this was added in a previous release (just find the major.minor version where it was added, I think it's still 3.14)
@@ -194,6 +194,10 @@ And:: | |||
Default value of *max_workers* is changed to | |||
``min(32, (os.process_cpu_count() or 1) + 4)``. | |||
|
|||
.. versionchanged:: next | |||
Added *ctxkwargs* to pass additional arguments to ``cls.prepare_context`` |
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.
What's prepare_context
? it doesn't help when we refer to functions without any docs.
@@ -166,7 +166,7 @@ interpreter objects as well as the following additions. | |||
Print an exit message when exiting. | |||
|
|||
|
|||
.. method:: InteractiveConsole.push(line) | |||
.. method:: InteractiveConsole.push(line, filename=None, _symbol="single") |
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.
I think we should have a separate PR for re-documenting InteractiveConsole
methods. In addition, I'm still unsure whether private parameters should be exposed or not. Personally, I would rather prefer not having them exposed but if subclasses need to override the method, I think their signature should then be compatible. Note that https://github.com/python/typeshed/blob/1c17cd429c2f91b0066547deb99c537af3e54d39/stdlib/code.pyi#L23-L37 does not expose this private parameter, so I think we shouldn't as well (and let this inconsistency with the signature remain; mypy wouldn't complain as typeshed also doesn't specify it.
Finally, filename
seems to be only 3.13+ so we should also be careful with the backports (therefore I'd prefer having a separate PR for InteractiveConsole
)
@@ -147,7 +147,7 @@ And:: | |||
executor.submit(wait_on_future) | |||
|
|||
|
|||
.. class:: ThreadPoolExecutor(max_workers=None, thread_name_prefix='', initializer=None, initargs=()) | |||
.. class:: ThreadPoolExecutor(max_workers=None, thread_name_prefix='', initializer=None, initargs=(), **ctxkwargs) |
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.
As it's 3.14+, this means that this should not be backported. So a separate PR is easier to handle as well. And we can make it part of gh-124694 instead.
📚 Documentation preview 📚: https://cpython-previews--129669.org.readthedocs.build/