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

json functions have too many positional parameters #62926

Closed
serhiy-storchaka opened this issue Aug 13, 2013 · 17 comments
Closed

json functions have too many positional parameters #62926

serhiy-storchaka opened this issue Aug 13, 2013 · 17 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Aug 13, 2013

BPO 18726
Nosy @gvanrossum, @rhettinger, @etrepum, @pitrou, @ezio-melotti, @bitdancer, @serhiy-storchaka
Files
  • json_keyword_only.patch
  • 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 = 'https://github.com/serhiy-storchaka'
    closed_at = <Date 2016-07-01.06:10:50.807>
    created_at = <Date 2013-08-13.11:18:14.722>
    labels = ['type-feature', 'library']
    title = 'json functions have too many positional parameters'
    updated_at = <Date 2016-07-01.06:10:50.806>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2016-07-01.06:10:50.806>
    actor = 'serhiy.storchaka'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2016-07-01.06:10:50.807>
    closer = 'serhiy.storchaka'
    components = ['Library (Lib)']
    creation = <Date 2013-08-13.11:18:14.722>
    creator = 'serhiy.storchaka'
    dependencies = []
    files = ['42759']
    hgrepos = []
    issue_num = 18726
    keywords = ['patch']
    message_count = 17.0
    messages = ['195068', '195070', '195071', '195072', '196712', '196717', '196747', '196751', '196791', '196792', '196799', '196805', '196806', '264991', '264993', '264996', '269024']
    nosy_count = 9.0
    nosy_names = ['gvanrossum', 'rhettinger', 'bob.ippolito', 'pitrou', 'ezio.melotti', 'Arfrever', 'r.david.murray', 'python-dev', 'serhiy.storchaka']
    pr_nums = []
    priority = 'low'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue18726'
    versions = ['Python 3.6']

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Aug 13, 2013

    The json module functions have too many positional parameters:

    dump() -- 11
    dumps() -- 10
    load() -- 9
    loads() -- 8

    In most time only small part of these options is specified so users call these functions with keyword arguments for all parameters except mandatory ones.

    Even worse, the simplejson module (an ancestor and an alternative to standard json module) have a very similar interface but with difference sequences of parameters (the second parameter of loads() and the ninth parameter of dumps() in simplejson is encoding). So in any case portable application should specify all but basic arguments as keyword arguments. If json will incorporate some features from simplejson in future positions of new parameters will be different.

    I propose to deprecate specifying all but mandatory parameters of json functions as positional arguments in 3.4 and then forbid it in 3.5.

    I.e. dumps() should be implemented in 3.4 as:

    def dumps(obj, *args, **kwargs):
        if args:
            warnings.warn("The positional arguments are deprecated.",
                         DeprecationWarning, stacklevel=2)
        return _dumps(obj, *args, **kwargs)
    
    def _dumps(obj, skipkeys=False, ensure_ascii=True, check_circular=True,
            allow_nan=True, cls=None, indent=None, separators=None,
            default=None, sort_keys=False, **kwargs):
        ...

    and in 3.5 as:

    def dumps(obj, *, skipkeys=False, ensure_ascii=True, check_circular=True,
            allow_nan=True, cls=None, indent=None, separators=None,
            default=None, sort_keys=False, **kwargs):
        ...

    @serhiy-storchaka serhiy-storchaka added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Aug 13, 2013
    @bitdancer
    Copy link
    Member

    bitdancer commented Aug 13, 2013

    This is not what we use keyword only arguments for. The standard practice in the stdlib is that arguments are arguments unless there is a good reason to make one keyword only. So I'm -1 on this proposal.

    @ezio-melotti
    Copy link
    Member

    ezio-melotti commented Aug 13, 2013

    Serhiy has a point though, I wouldn't want to see something like json.dumps(someobj, True, False, True, False).

    @bitdancer
    Copy link
    Member

    bitdancer commented Aug 13, 2013

    Ach. I didn't read carefully enough (not awake yet, I guess).

    Yes, boolean parameters are one of the things keyword only arguments are appropriate for.

    @rhettinger
    Copy link
    Contributor

    rhettinger commented Sep 1, 2013

    -1 Once the positional arguments are in the wild, you can't take them away without breaking a lot of code.

    This code was available as a third-party module prior to inclusion in Python and had many happy users. If the positional arguments had been a sticking point, it would have been known long ago.

    The time to express these concerns is when a module is being added. Afterwards, the API churn just isn't worth it. We don't want to create more obstacles to upgrading or converting from Python 2.

    @rhettinger rhettinger self-assigned this Sep 1, 2013
    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Sep 1, 2013

    We don't want to create more obstacles to upgrading or converting from Python 2.

    But these obstacles already exists.

    The 10th parameter of dump() is "encoding" in Python 2 and simplejson, and "default" in Python 3.

    The 12th parameter of dump() is "use_decimal" simplejson, and "sort_keys" in Python 2.

    The 2nd parameter of load() is "encoding" in Python 2 and simplejson, and "cls" in Python 3.

    Due to the fact that most arguments are boolean or accepts None some shifting of positional arguments can left unnoticed long time and causes subtle bugs when upgrading from Python 2 to Python 3 or switching between stdlib json module and simplejson.

    Proposed change doesn't break any code at first stage, it just adds deprecation warning about using dangerous non-portable feature. We can defer forbidding positional parameters until Python 2 will gone and simplejson will be changed to keyword-only parameters too.

    @rhettinger
    Copy link
    Contributor

    rhettinger commented Sep 1, 2013

    Serhiy, please resist the urge to engage in API churn. We've had zero user requests to abandon the current API. I'm not a fan of the current positional arguments, but changing it isn't really worth it (creating disruption with nearly zero benefit, no new functionality and possibly slowing down the calls in module where people seem to really care about speed).

    Bob, would you please weigh-in on this one. You have the best appreciation for the needs of the established user base.

    @rhettinger rhettinger assigned etrepum and unassigned rhettinger Sep 1, 2013
    @etrepum
    Copy link
    Mannequin

    etrepum mannequin commented Sep 1, 2013

    I tend to agree with Raymond here. While I also don't like the positional arguments, I don't think the benefit of API churn them is high enough to remove them. Other than this issue, I have not seen any requests for this change.

    I have seen problems because there are positional arguments, but only when people are subclassing these classes. Allowing for subclasses was never a good idea, and is actively discouraged in current simplejson documentation. The dumps default/loads object_hook functionality is sufficient and doesn't have the fragile base class problem. This functionality is is fully compatible with all versions of the library that are in use, back to v1.8 in March 2008, including Python 2.6's json library which IIRC is based on v1.9.

    @etrepum etrepum mannequin assigned rhettinger and unassigned etrepum Sep 1, 2013
    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Sep 2, 2013

    But what you will do with discrepancies between Python 2, Python 3 and simplejson?

    @ezio-melotti
    Copy link
    Member

    ezio-melotti commented Sep 2, 2013

    I have not seen any requests for this change.

    That's probably because either people have not realized that their code might be buggy, or because they have not realized that keyword-only args might be a solution to this problem (or maybe they did but didn't bother suggesting it).

    @etrepum
    Copy link
    Mannequin

    etrepum mannequin commented Sep 2, 2013

    Other than when subclassing (which is actively discouraged), I haven't seen anyone try and use positional args for these APIs. I simply don't think this is an issue in practice. If you go far enough back in simplejson history, these module-global functions were actually keyword-only.

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Sep 2, 2013

    If no one uses positional arguments then converting parameters to keyword-only will not break any code.

    @etrepum
    Copy link
    Mannequin

    etrepum mannequin commented Sep 2, 2013

    My evidence is only anecdotal. I haven't actively searched for code that uses json/simplejson to try and find cases that are using positional arguments.

    One way to test this assumption would be to release a version of simplejson that deprecates using positional arguments and see if anyone notices. If you feel strongly about it, submit a pull request, and I'll review it (although my availability is spotty for the next two months due to travel).

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented May 6, 2016

    Guido's decision on similar bpo-25628 is that changing keyword-or-positional parameters to keyword-only is safe and can be made without deprecation.

    If we started the deprecation period in 3.4, we could finish it in 3.6.

    Proposed simple patch changes all optional parameters in functions and constructors that takes too much parameters in the json module to keyword-only without deprecation.

    @gvanrossum
    Copy link
    Member

    gvanrossum commented May 6, 2016

    Guido's decision on similar bpo-25628 is that changing keyword-or-positional parameters to keyword-only is safe and can be made without deprecation.

    If we started the deprecation period in 3.4, we could finish it in 3.6.

    That makes little sense. You can't start deprecation in 3.4 or in 3.5 since these have been released already.

    But what I said implies that you can make these things mandatory in 3.6 without having to worry about deprecating them, so the start of the deprecation period is meaningless.

    However I do encourage you to be conservative, and add a warning about this and similar cases when the alphas and betas go out; and if you get much pushback during beta consider changing your mind.

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented May 6, 2016

    My remark was just about that I proposed to start a deprecation period at the time of developing Python 3.4. If this did accepted, the deprecation period would already finished and we would come up to the same code in 3.6.

    It doesn't matter now.

    @rhettinger rhettinger removed their assignment May 7, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 21, 2016

    New changeset db5fe5c4d09d by Serhiy Storchaka in branch 'default':
    Issue bpo-18726: All optional parameters of the dump(), dumps(),
    https://hg.python.org/cpython/rev/db5fe5c4d09d

    @serhiy-storchaka serhiy-storchaka self-assigned this Jul 1, 2016
    @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
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants