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

SimpleXMLRPCServer.SimpleXMLRPCServer.register_function as decorator #52017

Closed
ahojnnes mannequin opened this issue Jan 24, 2010 · 21 comments
Closed

SimpleXMLRPCServer.SimpleXMLRPCServer.register_function as decorator #52017

ahojnnes mannequin opened this issue Jan 24, 2010 · 21 comments
Assignees
Labels
3.7 stdlib type-feature

Comments

@ahojnnes
Copy link
Mannequin

@ahojnnes ahojnnes mannequin commented Jan 24, 2010

BPO 7769
Nosy @loewis, @rhettinger, @PCManticore, @berkerpeksag, @serhiy-storchaka, @zhangyangyu
PRs
  • #231
  • #703
  • #552
  • Files
  • SimpleXMLRPCServer.py: SimpleXMLRPCServer.py
  • xmlrpc_register_decorator_py27.patch: register_function as a decorator and unittest usage
  • xmlrpc_register_decorator_py33.patch: decorator implementation and unittest usage
  • issue7769.patch
  • issue7769_with_more_doc.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/zhangyangyu'
    closed_at = <Date 2017-02-28.09:15:31.500>
    created_at = <Date 2010-01-24.12:48:44.749>
    labels = ['3.7', 'type-feature', 'library']
    title = 'SimpleXMLRPCServer.SimpleXMLRPCServer.register_function as decorator'
    updated_at = <Date 2017-03-31.16:36:30.456>
    user = 'https://bugs.python.org/ahojnnes'

    bugs.python.org fields:

    activity = <Date 2017-03-31.16:36:30.456>
    actor = 'dstufft'
    assignee = 'xiang.zhang'
    closed = True
    closed_date = <Date 2017-02-28.09:15:31.500>
    closer = 'xiang.zhang'
    components = ['Library (Lib)']
    creation = <Date 2010-01-24.12:48:44.749>
    creator = 'ahojnnes'
    dependencies = []
    files = ['15986', '21057', '21058', '43401', '43655']
    hgrepos = []
    issue_num = 7769
    keywords = ['patch']
    message_count = 21.0
    messages = ['98219', '98230', '98233', '98236', '98238', '98239', '98241', '130403', '130406', '130411', '130433', '266798', '268620', '269875', '269941', '269952', '274777', '288692', '288696', '288701', '290359']
    nosy_count = 8.0
    nosy_names = ['loewis', 'rhettinger', 'ahojnnes', 'santoso.wijaya', 'Claudiu.Popa', 'berker.peksag', 'serhiy.storchaka', 'xiang.zhang']
    pr_nums = ['231', '703', '552']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue7769'
    versions = ['Python 3.7']

    @ahojnnes
    Copy link
    Mannequin Author

    @ahojnnes ahojnnes mannequin commented Jan 24, 2010

    I would suggest to make SimpleXMLRPCServer.SimpleXMLRPCServer.register_function a decorator function.
    See the attached file for the solution I wrote (l.209-240), which also works with the current syntax:
    @server.register_function
    @server.register_function('name')
    @server.register_function(name='name')
    or:
    server.register_function(func)
    server.register_function(func, name='name')
    server.register_function(function=func, name='name')

    So as far as I've tested it (py2.6), it is fully backwards compatible and supports decorators in addition.

    @ahojnnes ahojnnes mannequin added extension-modules type-feature labels Jan 24, 2010
    @briancurtin
    Copy link
    Member

    @briancurtin briancurtin commented Jan 24, 2010

    Can you submit your patch as a unified diff -- it's not easy to tell where you made changes to that file.
    Can you also include tests showing how this is used, especially using register_function in both the old way and the new way you propose? Lib/test/test_xmlrpc.py is a good place to put your tests.

    @ahojnnes
    Copy link
    Mannequin Author

    @ahojnnes ahojnnes mannequin commented Jan 24, 2010

    I'm not very used to working with bug/issue trackers, is there any tutorial here, where this is explained?

    I did the stuff you asked me to do:

    diff: http://paste.pocoo.org/compare/169357/169359/
    test: http://paste.pocoo.org/show/169360/

    @briancurtin
    Copy link
    Member

    @briancurtin briancurtin commented Jan 24, 2010

    http://www.python.org/dev/workflow/ and http://python.org/dev/faq/ should help you get started. The workflow doc will let you know what the process is around here. The FAQ will tell you how to setup Subversion, how to make a diff, etc.

    With that said, I think your change could be useful. Being that it's a feature request, it will go into 2.7 if accepted.

    @ahojnnes
    Copy link
    Mannequin Author

    @ahojnnes ahojnnes mannequin commented Jan 24, 2010

    OK, thank you for the links!

    Do you still want me to do anything (like test cases etc.)?

    @briancurtin
    Copy link
    Member

    @briancurtin briancurtin commented Jan 24, 2010

    Once you get a Subversion checkout of the source, have a look in Lib/test/test_xmlrpc.py for examples of how things are currently tested (using unittest). Once you get a feel for it, add new tests for what your changes do. The file you paste'd looks good as an example, you just have to make actual tests out of it now.

    @ahojnnes
    Copy link
    Mannequin Author

    @ahojnnes ahojnnes mannequin commented Jan 24, 2010

    OK, will work on it and reply as soon as I have results!

    @santosowijaya
    Copy link
    Mannequin

    @santosowijaya santosowijaya mannequin commented Mar 9, 2011

    I'm attaching a patch against 2.7 tip for an initial implementation of this decorator feature as well as sample usage in unittest, to get the ball rolling.

    The modified function should work as a decorator while preserving backward compatibility to be used in a traditional method call.

    @santosowijaya santosowijaya mannequin added stdlib and removed extension-modules labels Mar 9, 2011
    @briancurtin
    Copy link
    Member

    @briancurtin briancurtin commented Mar 9, 2011

    Santoso - since this is a feature request it would need to be retargeted to 3.3

    @santosowijaya
    Copy link
    Mannequin

    @santosowijaya santosowijaya mannequin commented Mar 9, 2011

    I see. Attaching a patch against 3.3 tip, then.

    @ahojnnes
    Copy link
    Mannequin Author

    @ahojnnes ahojnnes mannequin commented Mar 9, 2011

    sorry, I totally forgot about this...

    @berkerpeksag
    Copy link
    Member

    @berkerpeksag berkerpeksag commented Jun 1, 2016

    Here's some quick review comments:

    • xmlrpc_register_decorator_py33.patch doesn't apply cleanly anymore.

      •    serv.register_function(my_function)
        

      + serv.register_function(_my_function, name='my_function')

      We should keep serv.register_function(my_function) as is and add a separate line that uses the new 'name' parameter.

    • We can now change set([...]) to {...] in test_introspection1

      •        add_result, pow_result, div_result, \\
        

      + myfunc_result, myfunc2_result = multicall()

      No need to use the \ character.

    • The docstring of Lib/xmlrpc/server.py is already too long. It's better not to update it.

    • Please use the existing code style (name = None -> name=None)

    • We can make register_function act both as a decorator and as a decorator factory without changing its signature.

    • We need to add a test to cover TypeError case.

    • Documentation is missing. See Doc/library/xmlrpc.server.rst.

    • Please add a note to Doc/whatsnew/3.6.rst.

    @zhangyangyu
    Copy link
    Member

    @zhangyangyu zhangyangyu commented Jun 15, 2016

    Hi, I've written a patch to accomplish this in Py3.6.

    This patch is much cleaner but has one drawback, when used as decorator factory, you have to specify name as a keyword argument. But considering the codes that have to been imported to check arguments, I think it's not bad. (I don't want to check function is a string or not.)

    Also one side effect that I can not eliminate is when it is used as a normal function, the function instead of None is returned. I see the former patches get this problem too.

    Hope to get feedback.

    @zhangyangyu
    Copy link
    Member

    @zhangyangyu zhangyangyu commented Jul 6, 2016

    ping

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Jul 7, 2016

    I don't have an opinion about needing this feature. But for accepting it needs more documenting. The signature should be updated, the new feature should be documented in the main text, not only in the versionchanged note. Needed examples of using egister_function as decorator.

    @zhangyangyu
    Copy link
    Member

    @zhangyangyu zhangyangyu commented Jul 7, 2016

    From the old comments from Brian it seems he's willing to accept such a feature. But unfortunately Brian seems not interested in this now. IMHO, this is good feature especially to people familiar with Bottle and Flask. Writing in a decorator way seems more elegant to me. Nevertheless, add more doc.

    @zhangyangyu
    Copy link
    Member

    @zhangyangyu zhangyangyu commented Sep 7, 2016

    Also one side effect that I can not eliminate is when it is used as a normal function, the function instead of None is returned. I see the former patches get this problem too.

    I think this does not matter after seeing functools.singledispatch. The versionadded tags should be changed to versionchanged in my last patch.

    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented Feb 28, 2017

    +1 for the decorator idea. It feels very natural.

    A little off topic, I do not like the with-statement example that we currently have in the docs. I think it is bad design to put some much code inside with-block. In the micro-webframeworks, we register functions separately from launching the server. The same practice should apply here as well.

    @zhangyangyu
    Copy link
    Member

    @zhangyangyu zhangyangyu commented Feb 28, 2017

    Thanks Raymond. I would like to keep the example style another issue, not this one. :-) And I hope someone is willing to review the PR.

    @zhangyangyu zhangyangyu self-assigned this Feb 28, 2017
    @zhangyangyu
    Copy link
    Member

    @zhangyangyu zhangyangyu commented Feb 28, 2017

    Thanks everyone involved!

    @zhangyangyu
    Copy link
    Member

    @zhangyangyu zhangyangyu commented Mar 24, 2017

    New changeset 267b9d2 by Xiang Zhang in branch 'master':
    bpo-7769: enable xmlrpc.server.SimpleXMLRPCDispatcher.register_function used as decorator (GH-231)
    267b9d2

    @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
    3.7 stdlib type-feature
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants