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

Jump to bottom

pickle: inconsistent arguments pickle.py vs _pickle.c vs docs #83616

Closed
crusaderky mannequin opened this issue Jan 23, 2020 · 8 comments
Closed

pickle: inconsistent arguments pickle.py vs _pickle.c vs docs #83616

crusaderky mannequin opened this issue Jan 23, 2020 · 8 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes docs Documentation in the Doc dir stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@crusaderky
Copy link
Mannequin

crusaderky mannequin commented Jan 23, 2020

BPO 39435
Nosy @pitrou, @serhiy-storchaka, @miss-islington, @crusaderky
PRs
  • bpo-39435: Fix docs for pickle.loads #18160
  • [3.8] bpo-39435: Fix docs for pickle.loads (GH-18160) #19843
  • [3.7] bpo-39435: Fix docs for pickle.loads (GH-18160). #19844
  • bpo-39435: Make the first argument of pickle.loads() positional-only. #19846
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
closed_at = <Date 2020-05-01.19:59:45.010>
created_at = <Date 2020-01-23.17:12:19.406>
labels = ['3.7', '3.8', '3.9', 'type-feature', 'library', 'docs']
title = 'pickle: inconsistent arguments pickle.py vs _pickle.c vs docs'
updated_at = <Date 2020-05-02.06:38:08.659>
user = 'https://github.com/crusaderky'

    bugs.python.org fields:

    activity = <Date 2020-05-02.06:38:08.659>
actor = 'serhiy.storchaka'
assignee = 'docs@python'
closed = True
closed_date = <Date 2020-05-01.19:59:45.010>
closer = 'pitrou'
components = ['Documentation', 'Library (Lib)']
creation = <Date 2020-01-23.17:12:19.406>
creator = 'crusaderky'
dependencies = []
files = []
hgrepos = []
issue_num = 39435
keywords = ['patch']
message_count = 8.0
messages = ['360569', '360643', '360678', '364188', '367876', '367877', '367878', '367917']
nosy_count = 5.0
nosy_names = ['pitrou', 'docs@python', 'serhiy.storchaka', 'miss-islington', 'crusaderky']
pr_nums = ['18160', '19843', '19844', '19846']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue39435'
versions = ['Python 3.7', 'Python 3.8', 'Python 3.9']
    @crusaderky
    Copy link
    Mannequin Author

    crusaderky mannequin commented Jan 23, 2020

    (1)
    In the documentation for loads(), the name for the first argument of loads is 'bytes_object'. The actual signature, both in pickle.py and _pickle.c, it is instead 'data'.

    (2)
    In the documentation and in pickle.py, the default value for the 'buffers' parameter is None. However, in _pickle.c, it is an empty tuple (); this is also reflected by running the interpreter:

    In [1]: inspect.signature(pickle.loads).parameters['buffers']
    Out[1]: <Parameter "buffers=()">

    Thanks to @hauntsaninja for spotting these in python/typeshed#3636

    @crusaderky crusaderky mannequin added the 3.8 only security fixes label Jan 23, 2020
    @crusaderky crusaderky mannequin assigned docspython Jan 23, 2020
    @crusaderky crusaderky mannequin added docs Documentation in the Doc dir stdlib Python modules in the Lib dir 3.8 only security fixes labels Jan 23, 2020
    @crusaderky crusaderky mannequin assigned docspython Jan 23, 2020
    @crusaderky crusaderky mannequin added docs Documentation in the Doc dir stdlib Python modules in the Lib dir labels Jan 23, 2020
    @pitrou
    Copy link
    Member

    pitrou commented Jan 24, 2020

    As mentioned on the attached PR, the first argument is positional, so it doesn't matter that the name in the docs is not the same as the name in the code.

    The name "bytes_object" makes it clear which type of object is accepted, which makes it a better fit IMHO than "data". Therefore, I'm going to close this issue.

    @pitrou pitrou closed this as completed Jan 24, 2020
    @pitrou pitrou added the invalid label Jan 24, 2020
    @pitrou pitrou closed this as completed Jan 24, 2020
    @pitrou pitrou added the invalid label Jan 24, 2020
    @pitrou
    Copy link
    Member

    pitrou commented Jan 25, 2020

    Reopening because it seems my reading of the doc was wrong (took a backslash for a regular slash :-o).

    @pitrou pitrou reopened this Jan 25, 2020
    @pitrou pitrou removed the invalid label Jan 25, 2020
    @pitrou pitrou reopened this Jan 25, 2020
    @pitrou pitrou removed the invalid label Jan 25, 2020
    @serhiy-storchaka
    Copy link
    Member

    In pickle.py the name of the first parameter is "s" (it was "str" in 2.7), not "data". And in the module docstring it is "string". So we have two options:

    1. Make the first parameter positional-only. This is not breaking change because the documentation and two implementations had four different names, so there was no official way to pass it by keyword.

    2. Make it positional-and-keyword parameter officially. All implementations, documentation and docstrings should be synchronized.

    What is more preferable? Do we need to pass the first argument to pickle.loads() by keyword?

    After resolving this issue we may want to revise other modules which have the loads() function.

    @serhiy-storchaka serhiy-storchaka added 3.9 only security fixes type-feature A feature request or enhancement and removed 3.8 only security fixes labels Mar 14, 2020
    @miss-islington
    Copy link
    Contributor

    New changeset 289842a by Shantanu in branch 'master':
    bpo-39435: Fix docs for pickle.loads (GH-18160)
    289842a

    @miss-islington
    Copy link
    Contributor

    New changeset 3859b1a by Antoine Pitrou in branch '3.7':
    [3.7] bpo-39435: Fix docs for pickle.loads (GH-18160). (GH-19844)
    3859b1a

    @miss-islington
    Copy link
    Contributor

    New changeset e058280 by Antoine Pitrou in branch '3.8':
    [3.8] bpo-39435: Fix docs for pickle.loads (GH-18160) (GH-19843)
    e058280

    @pitrou pitrou added the 3.7 (EOL) end of life label May 1, 2020
    @pitrou pitrou added the 3.8 only security fixes label May 1, 2020
    @pitrou pitrou closed this as completed May 1, 2020
    @pitrou pitrou added 3.7 (EOL) end of life 3.8 only security fixes labels May 1, 2020
    @pitrou pitrou closed this as completed May 1, 2020
    @serhiy-storchaka
    Copy link
    Member

    New changeset 531d1e5 by Serhiy Storchaka in branch 'master':
    bpo-39435: Make the first argument of pickle.loads() positional-only. (GH-19846)
    531d1e5

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes docs Documentation in the Doc dir stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    3 participants
    @pitrou @serhiy-storchaka @miss-islington