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

ABCMeta.register() should work as a decorator #55077

Closed
kerio mannequin opened this issue Jan 8, 2011 · 18 comments
Closed

ABCMeta.register() should work as a decorator #55077

kerio mannequin opened this issue Jan 8, 2011 · 18 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@kerio
Copy link
Mannequin

kerio mannequin commented Jan 8, 2011

BPO 10868
Nosy @gvanrossum, @rhettinger, @pitrou, @benjaminp, @merwok, @durban
Files
  • abcmeta_register_deco.diff: proposed patch
  • abcmeta_register_deco_with_docs.diff: proposed patch with docs and corrections
  • abcmeta_register_v3.diff
  • abcmeta_register_v4.diff
  • abcmeta_register_v4_plus_tests.diff
  • 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/merwok'
    closed_at = <Date 2011-02-24.18:18:29.362>
    created_at = <Date 2011-01-08.23:32:11.376>
    labels = ['type-feature', 'library']
    title = 'ABCMeta.register() should work as a decorator'
    updated_at = <Date 2011-02-26.16:32:09.426>
    user = 'https://bugs.python.org/kerio'

    bugs.python.org fields:

    activity = <Date 2011-02-26.16:32:09.426>
    actor = 'stutzbach'
    assignee = 'eric.araujo'
    closed = True
    closed_date = <Date 2011-02-24.18:18:29.362>
    closer = 'eric.araujo'
    components = ['Library (Lib)']
    creation = <Date 2011-01-08.23:32:11.376>
    creator = 'kerio'
    dependencies = []
    files = ['20314', '20315', '20316', '20335', '20337']
    hgrepos = []
    issue_num = 10868
    keywords = ['patch']
    message_count = 18.0
    messages = ['125804', '125806', '125807', '125810', '125814', '125816', '125905', '125907', '125909', '125912', '125915', '126003', '129298', '129300', '129310', '129521', '129536', '129571']
    nosy_count = 8.0
    nosy_names = ['gvanrossum', 'rhettinger', 'pitrou', 'benjamin.peterson', 'stutzbach', 'eric.araujo', 'daniel.urban', 'kerio']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue10868'
    versions = ['Python 3.3']

    @kerio
    Copy link
    Mannequin Author

    kerio mannequin commented Jan 8, 2011

    If we make ABCMeta.register() return the registered class, like atexit.register() does for the registered function, we can then use it as a decorator:

    Now:

    class Foo:
        ...
    ABarC.register(Foo)

    With this change:

    @ABarC.register
    class Foo:
        ...

    The only problem this would cause is in code that relies on ABCMeta.register() to return None; I can't think of a reason anyone would rely on this, but...

    @kerio kerio mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Jan 8, 2011
    @pitrou
    Copy link
    Member

    pitrou commented Jan 8, 2011

    Looks like a good idea to me.

    @merwok
    Copy link
    Member

    merwok commented Jan 8, 2011

    Thanks for the report and patch. Some comments:

    + """Register a virtual subclass of an ABC. Returns the said subclass."""
    Should be “Return”, for consistency.

    + return subclass # For usage as a decorator
    I’d move that to the docstring.

    Can you edit the docs (remember to use the versionchanged directive) too?

    @merwok merwok self-assigned this Jan 8, 2011
    @kerio
    Copy link
    Mannequin Author

    kerio mannequin commented Jan 9, 2011

    Ok, edited the docs too; hope I did everything right...

    @merwok
    Copy link
    Member

    merwok commented Jan 9, 2011

    Edited your patch to fix some nits. If there is no opposition, I’ll commit this to py3k when 3.2 is out.

    @rhettinger
    Copy link
    Contributor

    +1

    @durban
    Copy link
    Mannequin

    durban mannequin commented Jan 10, 2011

    There is another return statement earlier in the ABCMeta.register method. The patch probably should also modify that.

    @kerio
    Copy link
    Mannequin Author

    kerio mannequin commented Jan 10, 2011

    Whoops, corrected that - should I add some more tests for that too?

    @merwok
    Copy link
    Member

    merwok commented Jan 10, 2011

    You should, otherwise how would you prove it’s fixed? :)

    @kerio
    Copy link
    Mannequin Author

    kerio mannequin commented Jan 10, 2011

    Fair enough :)

    @merwok
    Copy link
    Member

    merwok commented Jan 10, 2011

    -            return  # Already a subclass
    +            return subclass # Already a subclass
    PEP 8 advises to put two spaces before inline comments, for readability.
     (There is no need to upload a new patch, I’ll change that before commit.)

    + self.assertIsInstahce(c, (A,))
    This is fixed in the newest version of your patch. Running tests before
    uploading diffs catches such errors :)

    @kerio
    Copy link
    Mannequin Author

    kerio mannequin commented Jan 11, 2011

    Yeah, I should've waited for the test to finish, but come on, "it was just a small change" :(

    Now I know why you should always test everything at least, sorry about that :)

    @merwok
    Copy link
    Member

    merwok commented Feb 24, 2011

    Committed to py3k as r88545. You’ll notice that I fixed the nesting of the versionchanged directive and that I changed my mind about “returns”. Thanks again!

    @merwok merwok closed this as completed Feb 24, 2011
    @stutzbach
    Copy link
    Mannequin

    stutzbach mannequin commented Feb 24, 2011

    In what use-cases would you want to call MyABC.register() when defining a class instead of inheriting from MyABC?

    I always thought of the register() as hack to make it possible to support types written in C, which can't inherit from the ABC.

    @merwok
    Copy link
    Member

    merwok commented Feb 24, 2011

    Someone may want to register with an ABC but not inherit methods or add a class to the mro. It’s always been allowed by the register method; the new decorator feature is just a very minor nicety on top of that.

    Edoardo, was your request motivated by a real use case where you didn’t want to inherit from the ABC?

    @kerio
    Copy link
    Mannequin Author

    kerio mannequin commented Feb 26, 2011

    Not really, but putting something in your inheritance lattice only to mark a class as providing an interface just doesn't seem right to me.

    @durban
    Copy link
    Mannequin

    durban mannequin commented Feb 26, 2011

    In what use-cases would you want to call MyABC.register() when defining
    a class instead of inheriting from MyABC?

    For example if you don't want to inherit a __dict__ for a tree-like data structure (see also bpo-11333).

    @stutzbach
    Copy link
    Mannequin

    stutzbach mannequin commented Feb 26, 2011

    Sounds good to me. Thanks for the clarifications and satisfying my curiosity! :-)

    @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

    3 participants