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

Variant definition APIs #37

Closed
ghost opened this issue May 25, 2018 · 4 comments
Closed

Variant definition APIs #37

ghost opened this issue May 25, 2018 · 4 comments
Labels

Comments

@ghost
Copy link

ghost commented May 25, 2018

Cool package! I really like the chain.from_iterable style API.

Looking through this made me think about a bunch of options for how this idea could be implemented. I was curious about what other definition APIs are available and how they stack up.

Do you think either of these options might be a good idea? I'm sure you've thought a good deal about this design so I'd be quite interested to hear!

Given

@variants.primary
def print_text(txt):
    print(txt)

Some options:

Current:

@print_text.variant('from_file')
def print_text(fobj):
    print_text(fobj.read())

Alt 1:

@variant('from_file')
def print_text(fobj):
    print_text(fobj.read())

Alt 2:

@variant(print_text)
def from_file(obj):
    print_text(fobj.read())
@pganssle
Copy link
Member

pganssle commented May 25, 2018

I don't care for Alt 1 because I don't like how it magically introspects the name to figure out which function is a function group. This is how multipledispatch handles it, which actually causes some problems (see #27, for example).

Alt 2 is interesting and has the advantage that it forces from_file to be a valid Python identifier, as opposed to the current API which silently allows invalid identifiers to be added to the function group:

>>> from variants import primary
>>> @primary
... def somefunc():
...     pass
... 
>>> @somefunc.variant('123variant')
... def somefunc():
...     print('Called 123 variant!')
... 
>>> somefunc.123variant()
  File "<stdin>", line 1
    somefunc.123variant()
               ^
SyntaxError: invalid syntax
>>> getattr(somefunc, '123variant')()
Called 123 variant!

This is not ideal, but who was going to do that anyway.

The reason I went with something closer to the property decorator's syntax is because in Alt 2, the name of the variant leaks into the defining scope:

>>> @primary
... def func():
...     pass
... 
>>> @func.variant('from_file')
... def from_file(fobj):
...     print('From file!')
... 
>>> func.from_file(...)
From file!
>>> from_file
<VariantFunction func>

This could cause a lot of subtle and irritating problems if people want to name a variant the same as something from the outer scope - depending on which was defined first, the variant might overwrite the other name. If we tried to be clever and delete the name from the outer scope when it wasn't necessary anymore (probably a bad idea already), we'd probably end up deleting the original name.

There is actually one alternate form that you didn't mention that could alleviate at least the "invalid identifier" problem, but I didn't use it because it felt way too magical:

@primary
def func():
    pass

@func.define_variant.from_file
def func():
    print('from file!')

This could be done by creating a proxy object called define_variant defined something like this:

class define_variant:
    def __init__(self, parent):
        self._parent = parent

    def __getattr__(self, name):
        return self._parent.variant(name)

@primary
def func():
    pass

func.define_variant = define_variant(func)

@func.define_variant.from_file
def func(fobj):
    print('from_file')

func.from_file(...)
# from_file

The actual code would put define_variant's definition within the @primary decorator, but I broke it out for illustration purposes.

@ghost
Copy link
Author

ghost commented May 31, 2018

Thanks for the thoughtful answer! Those are great points about not messing with the containing namespace etc.

For what it's worth, I'm not sure I'd mind introspecting to get the function name, though I haven't totally thought through the issues you mention with e.g. multipledispatch.

If that option were available, perhaps the API could use something like this, which I especially like because the definition syntax mirrors the calling syntax:

@variant(func.from_file)
def func():
    print('Running func.from_file')


func.from_file(file)

Thanks again, I'm excited to see where this project goes! : )

@pganssle
Copy link
Member

pganssle commented Jun 1, 2018

I am not really in favor of presenting more than one API for this at the moment, but the point of this library is really to provide an easy way to do this. I would be happy to see more libraries pop up that use a different API.

For now I will close this ticket, because I don't think I'm going to provide another mechanism for creating these, though if a lot of people really want the syntactic sugar to look different, I may be persuaded to provide alternative APIs.

@pganssle pganssle closed this as completed Jun 1, 2018
@pganssle
Copy link
Member

pganssle commented Jun 1, 2018

@apnewberry By the way, thanks so much for your interest in this project. It's good that this library is spurring some conversations, because it's really designed to give us cleaner APIs, so the API of the library itself definitely is a good topic of discussion!

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

No branches or pull requests

1 participant