gh-135041: Expand and update pickletools module docstrings
#135042
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AA-Turner
reviewed
Jun 2, 2025
Misc/NEWS.d/next/Documentation/2025-06-02-19-49-33.gh-issue-135041.Jt5D8K.rst
Outdated
Show resolved
Hide resolved
AA-Turner
changed the title
pickletools module docstrings
AA-Turner
reviewed
Jun 2, 2025
AA-Turner
reviewed
Jun 2, 2025
AA-Turner
reviewed
Jun 2, 2025
Comment on lines
-1976
to
-1981
| If not isinstance(callable, type), REDUCE complains unless the | ||
| callable has been registered with the copyreg module's | ||
| safe_constructors dict, or the callable has a magic | ||
| '__safe_for_unpickling__' attribute with a true value. I'm not sure | ||
| why it does this, but I've sure seen this complaint often enough when | ||
| I didn't want to <wink>. |
|
All commit authors signed the Contributor License Agreement. |
AA-Turner
reviewed
Jun 3, 2025
Add Oxford comma Co-authored-by: Adam Turner <[email protected]>
|
I believe that should be all the changes you requested, let me know if I've missed anything else. |
| Stack after: ... pydict | ||
| where pydict has been modified via pydict[key] = value. | ||
| where pydict has been modified via pydict[key] = value. Note that any |
There was a problem hiding this comment.
Should not we change "dict" to "mapping" in the description? And "list" to "sequence" in the description of APPEND?
Comment on lines
+1819
to
+1821
| The index of the memo object to push is given by the positive | ||
| newline-terminated decimal string following. BINGET and LONG_BINGET | ||
| are space-optimized versions. |
There was a problem hiding this comment.
String can be positive.
Lib/pickletools.py
Outdated
Comment on lines
1918
to
1922
| EXT1 has a 1-byte integer argument. This is used to index into | ||
| the inverted extension registry, which contains integer to tuple | ||
| mappings. The tuples have a length of two in the format of | ||
| '("module", "name")'. This tuple is then passed through find_class, | ||
| and the result is pushed onto the stack. |
There was a problem hiding this comment.
It mentions too much implementation details (the inverted extension registry and its format). It should only refer to copyreg.add_extension() (which unfortunately is not documented) and find_class() (like in GLOBAL, or just refer to it).
Clarify the `APPENDS` docstring Co-authored-by: Serhiy Storchaka <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A few updates to the
pickletoolsdescription code strings (functioning as documentation for the Pickle Virtual Machine).