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

ENH: Adds excepts #262

Merged
merged 7 commits into from Jan 2, 2016
Merged

ENH: Adds excepts #262

merged 7 commits into from Jan 2, 2016

Conversation

llllllllll
Copy link
Contributor

The idea of this is to use exception based api functions alongside your normal functional code.

for example:

map(itemgetter('key'), seq) -> map(excepts(itemgetter('key'), KeyError), seq)

This helps us get around the fact that I cannot put an except clause in a lambda. I have found this to be very useful in my own code.

Most of this code is for fresh __name__ and __doc__ attributes.

@eriknw
Copy link
Member

eriknw commented Aug 7, 2015

Wouldn't this be more curry-able if the exceptions were the first argument?

@llllllllll
Copy link
Contributor Author

I hadn't considered that. The reason behind the order is I was mapping the syntax of a try/except into a call like:

try:
    f(*args, **kwargs)   # 1
except exc:  # 2
    default()  # 3

Also to match the rejected pep 463 http://legacy.python.org/dev/peps/pep-0463/

I think that the curry case (meaning it can be used as a decorator) far outweighs my reasons.

@llllllllll
Copy link
Contributor Author

pushed the flip change.

@llllllllll
Copy link
Contributor Author

Last commit was strange, apparently __name__ called on a class will not delegate to the descriptor but instances do. Looking into it, it appears that __name__ is a getset defined on PyType_Type and for all heaptypes it just returns the ht_name field. That is some strange behavior.

@llllllllll
Copy link
Contributor Author

ping @eriknw
No rush on this, just making sure we don't forget about this

@llllllllll
Copy link
Contributor Author

I made a slight api change:
Before, the third argument was default, a callble that would produce the default value.
Now, I have generalized this to be handler a callable that accepts the exception that was raised and returns the value of the expression. This is free to reraise if it would like.

This change makes this function a try/except analog to ifexprs.

@eriknw
Copy link
Member

eriknw commented Sep 16, 2015

Cool. I'll check this out (again) very soon. Somebody actually just asked me whether something like this exists. (btw, I do get pings. Sorry again for my delay. toolz is a pleasure to work on and I'm looking forward to getting back into things).

@ssanderson
Copy link
Contributor

Bump. I've wanted to use something like this in toolz-ish code a few times.

@mrocklin
Copy link
Member

mrocklin commented Dec 3, 2015

In general I like the idea.

The class within a class rings my (admittedly irrational) complexity warning bells. Is there a way to de-fancify that, perhaps using __wrapped__ as in #286

@llllllllll
Copy link
Contributor Author

I think the __wrapped__ would only let me forward the docstring of the underlying callable. We need to be able to generate this when requested. I didn't use a property because that tramples the class docstring.

@mrocklin
Copy link
Member

mrocklin commented Dec 3, 2015

Is the docstring of the underlying callable sufficient? If not then we
should probably also rethink #286.

On Wed, Dec 2, 2015 at 4:31 PM, Joe Jevnik notifications@github.com wrote:

I think the wrapped would only let me forward the docstring of the
underlying callable. We need to be able to generate this when requested. I
didn't use a property because that tramples the class docstring.


Reply to this email directly or view it on GitHub
#262 (comment).

@ssanderson
Copy link
Contributor

Is the docstring of the underlying callable sufficient? If not then we
should probably also rethink #286.

I think these are different use-cases. I expect that 99% of the usage of memoize, and probably 90% of the usage of curry is as decorators. In those cases, the fact that the function is curried and/or memoized should probably be in the function's docstring itself, which means that just showing the function docstring is sufficient.

By contrast, the primary use-case for excepts is when you want to embed a try/except block in an expression. That suggests that it's not often going to be used as a decorator, which means the documentation for the function isn't likely to describe the changed behavior, which means you probably want the modified behavior reflected in the docstring of the wrapper object.

@ssanderson
Copy link
Contributor

Also, I'm not sure if setting __wrapped__ changes where introspection tools look for docstrings. Idiomatic usage of functools.wraps sets the wrapper function's __doc__ independently of setting __wrapped__.

The primary use-case I had in mind for #286 was showing the source code via IPython's ?? operator.

@mrocklin
Copy link
Member

mrocklin commented Dec 3, 2015

OK, I guess my principle concern is the high-tech nature of generating and presenting the docstring. Is there a lower-tech solution?

@llllllllll
Copy link
Contributor Author

I don't know know if there is a way to preserve the class docstring on the class itself while still giving each instance a nice docstring explaining what the new function will do. If you have a suggestion I am open to changing the current implementation.


excepting = excepts(object(), object(), object())
assert excepting.__name__ == 'excepting'
assert excepting.__doc__ == excepts.__doc__
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to test that we only pass on the provided Exception type. A small test that raises a different kind of error than the provided type would be useful.

@mrocklin
Copy link
Member

I'm still not super happy about the dynamic class generation. I would not want to see this kind of solution percolate more broadly throughout toolz. I think that there are relatively few developers to whom it is an obvious approach (myself included) and so it reduces the development pool. However, I also recognize that I'm a bit dogmatic about this, and should relax from time to time.

So I suggest that we go ahead with what's here for now to see if perhaps my paranoia is unjustified.

@llllllllll
Copy link
Contributor Author

I can pull the inner class out of the body of excepts and just name it something like _Doc. You are not the only developer who thinks that this is an overly complicated solution so I can change the implementation.

@mrocklin
Copy link
Member

That sounds great :)

@llllllllll
Copy link
Contributor Author

I pulled the inner class out and added the test for exceptions not masked by excepts.

@llllllllll
Copy link
Contributor Author

ping @mrocklin

mrocklin added a commit that referenced this pull request Jan 2, 2016
@mrocklin mrocklin merged commit 1a451dd into pytoolz:master Jan 2, 2016
@mrocklin
Copy link
Member

mrocklin commented Jan 2, 2016

Merged. Thanks @llllllllll

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.

None yet

4 participants