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

Implementation of PEP 3102 Keyword Only Argument #43917

Closed
jiwon mannequin opened this issue Aug 31, 2006 · 14 comments
Closed

Implementation of PEP 3102 Keyword Only Argument #43917

jiwon mannequin opened this issue Aug 31, 2006 · 14 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs)

Comments

@jiwon
Copy link
Mannequin

jiwon mannequin commented Aug 31, 2006

BPO 1549670
Nosy @gvanrossum
Files
  • pep3102-final.patch: Now the compiler package correctly parses the grammar for pep3102.
  • 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/gvanrossum'
    closed_at = <Date 2008-01-06.22:29:46.443>
    created_at = <Date 2006-08-31.01:06:34.000>
    labels = ['interpreter-core']
    title = 'Implementation of PEP 3102 Keyword Only Argument'
    updated_at = <Date 2008-01-06.22:29:46.443>
    user = 'https://bugs.python.org/jiwon'

    bugs.python.org fields:

    activity = <Date 2008-01-06.22:29:46.443>
    actor = 'admin'
    assignee = 'gvanrossum'
    closed = True
    closed_date = None
    closer = None
    components = ['Interpreter Core']
    creation = <Date 2006-08-31.01:06:34.000>
    creator = 'jiwon'
    dependencies = []
    files = ['7515']
    hgrepos = []
    issue_num = 1549670
    keywords = ['patch']
    message_count = 14.0
    messages = ['51023', '51024', '51025', '51026', '51027', '51028', '51029', '51030', '51031', '51032', '51033', '51034', '51035', '51036']
    nosy_count = 3.0
    nosy_names = ['gvanrossum', 'nnorwitz', 'jiwon']
    pr_nums = []
    priority = 'normal'
    resolution = 'accepted'
    stage = None
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue1549670'
    versions = ['Python 3.0']

    @jiwon
    Copy link
    Mannequin Author

    jiwon mannequin commented Aug 31, 2006

    This patch is implementation of PEP-3102, keyword-only
    parameter.

    Important changes include

    • code object now has co_kwonlyargcount to keep the
      number of keyword only argument - this is analogous to
      co_argcount.
    • function object now has func_kwdefaults (dictionary)
      to keep the mapping between keyword only arguments and
      defaults for them. Only kwonly argument with default
      values are in the dictionary - this is analogous to
      func_defaults.
    • APIs for code object changed - both C API and Python
      Api. PyCode_New now takes number of keyword only
      arguments, and new.code also takes number of keyword
      only arguments.
    • MAKE_FUNCTION now takes another oparg, which is
      number of default keyword only arguments - and the name
      of keyword only argument and its default value are in
      the value stack - it is similar to oparg of CALL_FUNCTION.
    • MAGIC in import.c changed, since bytecode is changed.

    That's pretty much everything that's important, and the
    rest is in the code itself.

    And my patch passes all regression tests excepts
    test_frozen.py, which depends on the hard-coded mashal
    value, which needs to be regenerated when bytecode
    changes. However, freeze.py is broken - specifically,
    modulefinder.py is broken as Guido said. So, currently,
    I commented it out.

    @jiwon jiwon mannequin closed this as completed Aug 31, 2006
    @jiwon jiwon mannequin assigned gvanrossum Aug 31, 2006
    @jiwon jiwon mannequin added interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Aug 31, 2006
    @nnorwitz
    Copy link
    Mannequin

    nnorwitz mannequin commented Aug 31, 2006

    Logged In: YES
    user_id=33168

    import.c: comment has double word 'added'
    ast.c:

    • why is static commented out for ast_for_arguments?
    • in handle_keywordonly_args, doesn't the default case need
      to return -1 after setting the error?
    • need to change the // comments to /* */
    • it looks like a bunch of lines had whitespace, but no
      other changes. this makes it hard to see the real changes.

    compile.c:

    • i think there is a bug in the arglength calc if there are
      more than 255 positional args and kw_default_count since
      there is |= kw_default_count << 8 (2 places)
    • return should go on its own line like the rest of the
      surrounding code.
    • why kwonlyargs and kw_args? the underscore seems
      inconsistent

    in the compiler package, I didn't see a change to
    MAKE_FUNCTION similar to the one in compile.c for the opcode
    stack effect.

    the change to regrtest.py doesn't seem necessary (just an
    added blank line)

    there should be a lot more tests added for both positive and
    negative conditions (syntax errors). Include variants with
    func(**kwargs) func(*args), etc in calls.

    it's not good to comment out the tests in test_frozen.
    otherwise, we will probably forget about the problems.

    when you say it passes all tests, did you run with -u all?
    the compiler doesn't do all the tests without it (and one or
    two other tests too).

    in the new test, i think you need to pass a name for the
    filename param to compile. I think there was some platform
    (windows?) that crapped out with an empty filename. (if you
    copied from an existing test, then i must be remembering
    something different.)

    In testRaiseErrorWhenFuncall(), you can use
    self.assertRaises, at least for most of the cases.

    you need a test_main for this test to be run from regrtest.

    @jiwon
    Copy link
    Mannequin Author

    jiwon mannequin commented Aug 31, 2006

    Logged In: YES
    user_id=595483

    > * it looks like a bunch of lines had whitespace, but no
    > other changes. this makes it hard to see the real changes.

    I changed some tabs in ast_for_argument function to spaces -
    tabs and spaces are mixed, and it's hard to edit. It would
    have been nice if I separate that white space conversion
    into another patch, but since it's p3yk branch I thought it
    can be allowed.

    > when you say it passes all tests, did you run with -u all?
    > the compiler doesn't do all the tests without it (and one
    > or
    > two other tests too).

    Yup, it passes all the tests except test_frozen and
    test_linuxaudiodev.py(maybe because my box doesn't have
    audio device..?) and some skipped tests including
    test_bsddb3.py, etc

    > in the new test, i think you need to pass a name for the
    > filename param to compile. I think there was some platform
    > (windows?) that crapped out with an empty filename. (if you
    > copied from an existing test, then i must be remembering
    > something different.)

    Yeah, I copied it from test_with.py, but now I'm passing a
    filename.

    @jiwon
    Copy link
    Mannequin Author

    jiwon mannequin commented Aug 31, 2006

    Logged In: YES
    user_id=595483

    Maybe I should mention that I've uploaded new patch that
    fixed what Neal mentioned.

    @jiwon
    Copy link
    Mannequin Author

    jiwon mannequin commented Sep 7, 2006

    Logged In: YES
    user_id=595483

    The original patch crashes when a function has keyword only
    arguments but no default values for those arguments, and
    calls the function without enough arguments. Fixed the bug,
    and added testcases for that.

    @gvanrossum
    Copy link
    Member

    Logged In: YES
    user_id=6380

    General comment: I think you should update or add a comment
    explaining the contents of the code object as it has changed
    (unless that's all described in the PEP).

    There are a number of places where the code is wider than 79
    characters or where the indentatiln style seems to be off.
    You may have to set your tabstops to 8 spaces to see these.

    E.g.:

    ceval.c:

    • line 2624 should be indented with next line
    • line 2680 is too long
    • @@ -2694,13 +2715,35 @@ several lines too long
    • line 3618 should be indented with the next line

    ast.c: @@ -591,6 +591,63 @@ please follow the surrounding
    4-space indents; don't use tabs here

    marshal.c: line 871 too long

    codeobject.c: several lines too long

    funcobject.c: too long at "non-dict keyword only default args");

    Lib/compiler/*.py: several lines too long

    Lib/test/test_new.py

    Modules/pyexpat.c: @@ -279,6 +279,7 @@ indentation error

    Modules/parsermodule.c: several lines too long; the loop at
    "while (res && i+1 < nch)" is indented one tabstop too many
    (you seem to have edited this file with ts=4?)

    @jiwon
    Copy link
    Mannequin Author

    jiwon mannequin commented Sep 13, 2006

    Logged In: YES
    user_id=595483

    The codeobject has a kwonlyargcount meaning the number of
    keyword only arguments. Thus, the api for creating new code
    object is changed: PyCode_New now takes kwonlyargcount.

    So, the signature changes from this
    PyCode_New(argcount, nlocals, stacksize, ...)

    to following.
    PyCode_New(argcount, kwonlyargcount, nlocals, stacksize, ...)

    From python, you can access this with name
    "co_kwonlyargcount" such as, co.co_kwonlyargcount just like
    you access argcount with co.co_argcount.

    @jiwon
    Copy link
    Mannequin Author

    jiwon mannequin commented Sep 14, 2006

    Logged In: YES
    user_id=595483

    I made a big mistake with the grammar - currently, the
    grammar doesn't allow keyword only argument after *vararg.
    At some point of the development process, I just assumed
    as such. Underlying logic does not assume anything about
    that, so I'll try to fix it as soon as possible

    @gvanrossum
    Copy link
    Member

    Logged In: YES
    user_id=6380

    I see. The Grammar should simply make the NAME after the '*'
    optional. Shouldn't be too hard to fix should it?

    @jiwon
    Copy link
    Mannequin Author

    jiwon mannequin commented Oct 9, 2006

    Logged In: YES
    user_id=595483

    The new patch that I am uploading fixes the Grammar
    problem - originally I made mistake not allowing *vararg
    after keyword only argument. The python compiler package
    doesn't work now, but everything else should work now.

    @gvanrossum
    Copy link
    Member

    Logged In: YES
    user_id=6380

    Checked in as r52491.

    I'm keeping this open awaiting a patch for the compiler package.

    @jiwon
    Copy link
    Mannequin Author

    jiwon mannequin commented Nov 22, 2006

    Now the compiler package works. The patch passes regrtest with "-u all" option and "-u compiler" option (except test_frozen.py). The patch is against latest p3yk branch HEAD. The last patch accidentally included some test code in test_compiler.py (and which is committed to the svn) so this patch includes fix for that.
    Ah, Guido, when you committed the patch (previous version) last time, you did not add Lib/test/test_keywordonlyarg.py so, this time, don't forget to add it :) Also, last time when we talked, I think you said that the patch makes some test case fail on your laptop machine. You might want to check that.

    @gvanrossum
    Copy link
    Member

    Submitted.

    Jiwon, can you mail me another copy of test_keywordinoyarg.py? I believe I lost it.

    @gvanrossum
    Copy link
    Member

    All submitted. (test_keywordonlyarg.py too)

    @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
    interpreter-core (Objects, Python, Grammar, and Parser dirs)
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant