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

interface.implements decorator #20

Open
nicoddemus opened this issue Aug 23, 2018 · 4 comments
Open

interface.implements decorator #20

nicoddemus opened this issue Aug 23, 2018 · 4 comments

Comments

@nicoddemus
Copy link
Contributor

Hi @ssanderson, congratulations on the package!

We have an in-house of implementation of interfaces which is very similar to yours in philosophy and usage.

One key difference in our implementation is that we use a decorator to signal implementation instead of subclassing. For example:

class MyClass(implements(MyInterface)):

    def method1(self, x):
        return x * 2

    def method2(self, x, y):
        return x + y

In our framework is written as:

@implements(MyInterface)
class MyClass:

    def method1(self, x):
        return x * 2

    def method2(self, x, y):
        return x + y

On advantage is that we don't need to insert any classes in the user's hierarchy, similar to how attr.s works, which is less intrusive.

This is not really an issue but more of a question on what you think about the approach of using a decorator?

@ssanderson
Copy link
Owner

hey @nicoddemus.

The main reason I use subclassing here is that it allows me to guarantee that subclasses don't break interfaces. For example, if I write:

class SomeInterface(Interface):
    def method(self, x):
        pass

class MyImpl(implements(SomeInterface)):
    def method(self, x):  # correctly implemented
        pass

class MySubclass(MyImpl):
    def method(self, x, y):  # oops, this one is wrong
        pass

this package will raise an error telling you that MySubclass is failing to implement an interface that its parent promised to implement:

InvalidImplementation: 
class MySubclass failed to implement interface MyInterface:

The following methods of MyInterface were implemented with invalid signatures:
  - method(self, a, b) != method(self, a)

To get the same behavior with a decorator-based approach, you'd have to ensure that every subclass re-applies the same decorator. My general feeling is that the purpose of this library is to try to prevent these kinds of accidental mistakes, which I can do more effectively with the subclassing-based API.

The main downside to this strategy, as you note, is that my implementation requires insertion of an extra class, as well as a metaclass, which can cause conflicts if you're using another metaclass. You can generally work around this by making your metaclass a subclass of ImplementsMeta, but that's more metaprogramming than most people like to use for serious work. See #12 (comment) for a concrete example of this.

I'd be open to the idea of an alternate decorator-based API that used less type-based magic at the cost of not protecting subclasses.

@nicoddemus
Copy link
Contributor Author

Hey @ssanderson, thanks for the detailed explanation.

Indeed our implementation requires you to reapply the decorator in cases where subclasses change the method of a superclass. We have a pretty large codebase and did not miss this functionality, but YMMV.

We were planning on releasing our interface implementation as OS, and I found your project while looking for an available name on PyPI, so we are considering contributing to your project instead of releasing yet another "interface" implementation.

I believe we can add the decorator-based API in addition to the subclass-based one without conflict in the same codebase, if you are interested.

Let me know what you think.

@ssanderson
Copy link
Owner

ssanderson commented Aug 28, 2018

I'd be open to a PR that adds a decorator-based API. I think the functionality you want would basically just require extracting the core logic of ImplementsMeta.__new__ into a standalone function. The hardest question should just be bikeshedding over naming choices. I don't think there's a straightforward way to re-use the name implements. I've suggested check_implements elsewhere for this, but I'm not sure that's the right name given the notes below.

One piece of functionality that might be worth thinking about carefully is our support for defaults. Currently, if you decorate an interface method with @default, we copy the decorated function onto new types that subclass from implements(TheInterface), and we do some validation to ensure that types implementing multiple interfaces don't end up with conflicting default-provided methods. I think that functionality still makes sense for the decorator-based API, but depending on our naming choices I could imagine that being surprising behavior (e.g., if we named the decorator function something like check_implements, I wouldn't expect it to have side-effects).

@nicoddemus
Copy link
Contributor Author

I see, thanks @ssanderson for your considerations, appreciate it.

I will check with my coworkers how we can approach this. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants