Skip to content
This repository has been archived by the owner on Apr 22, 2020. It is now read-only.

Add the mirror_defaults decorator #185

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

idanarye
Copy link

@idanarye idanarye commented Aug 1, 2019

This can solve the problem of using AUTO vs replicating arguments. We could have:

def retry(times, func, args=[], kwargs={}, acceptable=Exception, sleep=1,
          max_sleep=False, log_level=logging.DEBUG, pred=None, unacceptable=()):
    ...


@mirror_defaults(retry)
retrying(times, acceptable=AUTO, sleep=AUTO, max_sleep=AUTO, log_level=AUTO, pred=AUTO, unacceptable=AUTO):
    ...

@idanarye
Copy link
Author

idanarye commented Aug 1, 2019

c.c. @koreno - I can't add you as a reviewer.

@idanarye idanarye force-pushed the add-mirror_defaults-decorator branch from ba3a10a to b453f3f Compare August 1, 2019 14:35
@idanarye idanarye force-pushed the add-mirror_defaults-decorator branch from b453f3f to d8b183b Compare August 1, 2019 15:03
@koreno
Copy link
Contributor

koreno commented Aug 1, 2019

  1. AUTO might be a desireable override. better make it mirror_defaults(foo, token=MIRROR) (allow using an alternate token, default to MIRROR)
  2. can this be applied in many other places in easypy? wait/iter_wait? resilient/resilience? if it works then we can consider incorporating this. Otherwise I'm feeling this might be too fringe and magic... (I haven't thought it thoroughly)

@koreno
Copy link
Contributor

koreno commented Aug 1, 2019

c.c. @koreno - I can't add you as a reviewer.

@doroncohen will I be re-admitted to this repo?...

@idanarye
Copy link
Author

idanarye commented Aug 1, 2019

  • AUTO might be a desireable override. better make it mirror_defaults(foo, token=MIRROR) (allow using an alternate token, default to MIRROR)

How do you feel about mirror_defaults being the default "token" (since it's already imported)?

  • can this be applied in many other places in easypy? wait/iter_wait? resilient/resilience? if it works then we can consider incorporating this. Otherwise I'm feeling this might be too fringe and magic... (I haven't thought it thoroughly)

I see no reason why it shouldn't work everywhere - though we may need to have special treatment for generators to avoid messing up reflection...

@doron-cohen
Copy link

I think that we should allow the users to use these features but using them in the repo might put off other developers from contributing.

@koreno
Copy link
Contributor

koreno commented Aug 4, 2019

I think that we should allow the users to use these features but using them in the repo might put off other developers from contributing.

I think that if we don't use an easypy feature within easypy, then it shouldn't be in easypy...
Either we sanction it and demonstrate its greatness in our code, or we deem it improper coding for some reason and therefor reject it.

  • AUTO might be a desireable override. better make it mirror_defaults(foo, token=MIRROR) (allow using an alternate token, default to MIRROR)

How do you feel about mirror_defaults being the default "token" (since it's already imported)?

mm.... that feels odd and unreadable. mirror_defaults.MIRROR?

  • can this be applied in many other places in easypy? wait/iter_wait? resilient/resilience? if it works then we can consider incorporating this. Otherwise I'm feeling this might be too fringe and magic... (I haven't thought it thoroughly)

I see no reason why it shouldn't work everywhere - though we may need to have special treatment for generators to avoid messing up reflection...

then I think we should implement this treatment...

@idanarye
Copy link
Author

idanarye commented Aug 4, 2019

mm.... that feels odd and unreadable. mirror_defaults.MIRROR?

Too long. Let's just use MIRROR - one could always import it together with mirror_defaults

@idanarye
Copy link
Author

idanarye commented Aug 4, 2019

Can we decide about the token before I start applying this wherever possible and using that token everywhere?

@YuvalEvron
Copy link
Contributor

YuvalEvron commented Aug 4, 2019

Can we decide about the token before I start applying this wherever possible and using that token everywhere?

We can also pass the names of parameters we want to mirror as *args to the decorator.

@idanarye
Copy link
Author

idanarye commented Aug 4, 2019

Another option is to create a decorator that verifies the defaults are the same:

def foo(a=1, b=2, c=3):
    ...


@ensure_same_defaults(foo)
def bar(a=1, b=2, c=4):
    ...

This will raise an exception because bar's default for c is 4 instead of 3. It's less automatic than mirror_defaults but it has the advantage of IPython and IDEs showing the correct signature.

@koreno
Copy link
Contributor

koreno commented Dec 23, 2019

So do we still want this? or did we lose interest?

@YuvalEvron
Copy link
Contributor

So do we still want this? or did we lose interest?

Looks like we can't agree on the implementation.

@idanarye
Copy link
Author

So do we still want this? or did we lose interest?

Looks like we can't agree on the implementation.

Not on the mirror_defaults implementation, but I think we can agree on the ensure_same_defaults implementation?

@YuvalEvron
Copy link
Contributor

So do we still want this? or did we lose interest?

Looks like we can't agree on the implementation.

Not on the mirror_defaults implementation, but I think we can agree on the ensure_same_defaults implementation?

👍

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

Successfully merging this pull request may close these issues.

4 participants