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

Better handling of callables #15

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

alexandre
Copy link

Hello,

Instead to use a function, I prefer to write a class aiming a better description of my analysis rules. So, I defined a callable:

class MyCallable(object):
    def __init__(self):
        # proof.Analysis will look for a dunder name attribute (duck typing)
        self.__name__ = self.__class__.__name__

    def __call__(self, data):
        # analysis rules

Then, I use it in the Analysis.then method:

import proof

analysis_one = MyCallable()
myproof = proof.Analysis(initial_data)

myproof.then(analysis_one)

The proof.Analisys._fingerprint is considering only functions and instead any callable like.

TL;DR I got this exception: https://github.com/python/cpython/blob/master/Lib/inspect.py#L624

I think this PR could be an initial step to a better way to handle callable in this method (_fingerprint).

P.S - I'm sorry if I broke some PR rule/template. I would be happy to discuss this point, improve this PR and write more tests if necessary.

@onyxfish
Copy link
Collaborator

Hi @alexandre, this looks great. It's an oversight on my part that callables aren't supported. I definitely intended for this to work. I'll merge this into the next release.

@alexandre
Copy link
Author

@onyxfish ,

I'm glad you did like. I could improve this pull request with more some tasks:

  • the attribute _func should has a more suggestive name. If implement the method __call__, can be used (if makes quak! == is a duck). So what about rename to _callable ?
  • would be great if the docs explain that you need implement an attribute __name__ if you are not using a function
  • for last, but no for that less important: an example in the docs about this alternative way.

What do you think? would be better I open an issue ?

@onyxfish
Copy link
Collaborator

Those changes sound great! Please feel free to go ahead and implement them on this pull request. No need to file another issue.

@alexandre
Copy link
Author

Ok! :]

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

Successfully merging this pull request may close these issues.

2 participants