-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Disallow .__call__() as workaround for non-named functions #32460
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what you are suggesting here? in theory lambda functions could be supported, but to be honest i think the error is fine here. Always start with a test.
It's not about lambdas. In this context "named" means a variable name or an attribute - pandas doesn't allow calling arbitrary expressions. For example |
0a3a36d
to
f8fe5ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs a whatsnew note, bug fixes for 1.1 under the reshaping section. pls merge master and ping on green.
|
||
df.eval("@func()") | ||
|
||
with pytest.raises(TypeError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you test the exception message as well, it is not clear what the user is going to see here
@jreback ping! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks ok, ping on green.
@@ -1181,3 +1181,17 @@ def test_failing_character_outside_range(self, df): | |||
def test_failing_hashtag(self, df): | |||
with pytest.raises(SyntaxError): | |||
df.query("`foo#bar` > 4") | |||
|
|||
def test_call_non_named_expression(self, df): | |||
def func(*_): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add the issue number here as a comment.
Hello @alexmojaki! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-05-10 17:52:29 UTC |
@jreback pong! |
thanks @alexmojaki |
Currently this script:
Fails with:
however this can easily be worked around by adding
.__call__
:I'm assuming we don't want to allow this workaround. This PR ensures that it will fail with the same error.