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

Feature Request: promised default values #115

Open
hodgespodge opened this issue Oct 3, 2023 · 9 comments
Open

Feature Request: promised default values #115

hodgespodge opened this issue Oct 3, 2023 · 9 comments

Comments

@hodgespodge
Copy link
Contributor

I'm sure this is a niche request, but I'm running into a situation where I would like to use PromisedTypes with default values.

Here is an example of the problem I'm faced with:

myClass = PromisedType("myClass")

class exampleClass:
    def __init__(self):
        from myClass import myClass as myClassImport
        myClass.deliver(myClassImport)

    @dispatch
    def test(self, value: myClass = myClass()): # For example, here I want the default value to be myClass's parameterless constructor
        ...

If support for the above code isn't feasible, having some sort of mechanism by which one could define a promised value could work too:

myClass = PromisedType("myClass")
myClassDefault = PromisedValue("myClass")

class exampleClass:
    def __init__(self):
        from myClass import myClass as myClassImport
        myClass.deliver(myClassImport)
        myClassDefault.deliver(myClassImport())

    @dispatch
    def test(self, value: myClass = myClassDefault): 
        ...
@PhilipVinc
Copy link
Collaborator

PhilipVinc commented Oct 3, 2023

Wouldmt an easier workaround be to declare an additional dispatch with no arguments, importing the actual type and calling the function itself with an instance of the type?

This should address your use case.

@dispatch
def test(self):
    import mytype
    return self.test(mytype())

@hodgespodge
Copy link
Contributor Author

Thanks for weighing in @PhilipVinc
You're totally correct. That is a good workaround for the example I posted.

My preference towards full support for default arguments is motivated by a couple things:
Plum provides excellent support for kwargs as long as they have default values. This is especially true for functions with multiple default values.

Also, I think being able to read and understand a function's arguments quickly is a really nice thing provided by default values.

@wesselb
Copy link
Member

wesselb commented Oct 4, 2023

Hey @hodgespodge, thanks for opening an issue. :)

This is a very interesting proposal! I suppose that the default value would be some kind of self-mutating object, or Plum would have to do some logic to automatically replace the placeholder object whenever the class becomes available.

Although this would technically be possible, what gives me pause for thought is that it would add a lot of magic. I tend to prefer to keep things as simple as possible, unless the additional complexity really is justified.

@PhilipVinc's trick is a very decent good workaround!

Another less sexy workaround is to use Optional, though I think @PhilipVinc's approach is better:

def f(x: Optional[MyPromisedClass] = None):
    if x is None:
        from package import MyClass
        x = MyClass()
    ...

How would it sound to keep the issue open for a bit to see what others think? Implementing this would not be straightforward, so I think it's worthwhile to consider the pros and cons.

@PhilipVinc
Copy link
Collaborator

I played around with this a bit, making a promised value is easy (https://github.com/PhilipVinc/plum/tree/pv/promised_val) the main complication is that right now we simply register the same method for different signatures, but that won't work.

Instead, I think what could work is that the 'default method' would be different from the original one, and would replace the promised value object with the actual one.

It's not an hard change to do. I'm a bit unhappy with it, but would be doable.

@PhilipVinc
Copy link
Collaborator

I think implementing this is not particularly complex. It mainly adds more logic to promised types that when delivered would need to deliver to all promised values, and then a logic to generate methods that replace the arguments.

This is also relatively easy to do.

But as @wesselb says, it seems like a lot of magic/complexity that could be solved by just importing things in the right order?

@hodgespodge
Copy link
Contributor Author

@PhilipVinc

But as @wesselb says, it seems like a lot of magic/complexity that could be solved by just importing things in the right order?

Totally understandable if it seems like more trouble than its worth. I'm working on autogenerating python wrappers for C# code hence why we run into circular imports at all. Could provide a very nice solution for a problem which is admittedly not very common.

@wesselb
Copy link
Member

wesselb commented Oct 10, 2023

@PhilipVinc your attempt at PromisedValue seems very reasonable. I'm mainly worried by the logic that would have to replace to automatically replace the default value. One approach would be to automatically apply @PhilipVinc's suggestion, i.e. automatically split

@dispatch
def f(x: A = A(*args, **kw_args)):
    ...

into

@dispatch
def f(x: A):
    ...


@dispatch
def f():
    return f(A.resolve()(*args, **kw_args))

IMO, that looks like a reasonably clean way to go about it.

@PhilipVinc
Copy link
Collaborator

PhilipVinc commented Oct 10, 2023

Yes. One would only have to change this line registering the method to change the method corresponding to the sub signature to one constructed to call resolve instead of the same method.

(Note the shameless plug about my PR. In the current code it would require changing the sub signature which would be the same thing but a bit less explicit)

@wesselb
Copy link
Member

wesselb commented Oct 11, 2023

@PhilipVinc that's awesome! I'm sorry for being slow lately. It's sometimes hard to find enough time to work on this. I'm trying to get through things in order of priority, first #119, which I think we should be able to merge very soon!

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

3 participants