Skip to content

CLN: parts of #29667 #29677

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

Merged
merged 2 commits into from
Nov 18, 2019
Merged

CLN: parts of #29667 #29677

merged 2 commits into from
Nov 18, 2019

Conversation

jbrockmendel
Copy link
Member

Breaks off easier parts

@jreback jreback added the Clean label Nov 17, 2019
@@ -197,7 +197,9 @@ class Op:
Hold an operator of arbitrary arity.
"""

def __init__(self, op, operands, *args, **kwargs):
op: str
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a typo?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, there is a op attribute that is a str

Copy link
Member

Choose a reason for hiding this comment

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

is it necessary?

reveal_type(_bool_op_map)
reveal_type(_bool_op_map.get("not", "not"))
pandas\core\computation\ops.py:193: note: Revealed type is 'builtins.dict[builtins.str*, builtins.str*]'
pandas\core\computation\ops.py:194: note: Revealed type is 'builtins.str'

Copy link
Member Author

Choose a reason for hiding this comment

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

is it necessary?

i guess im not clear on the policy. i thought it was a "more is better" kind of thing

Copy link
Member

Choose a reason for hiding this comment

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

I'm just suspicious when I see unnecessary variable annotations since they could be casts in disguise.

In this case, it could have been (but wasn't) hiding that the return type of _bool_op_map.get(op,op) might be Any.

If the return type of some_function is Any and we do some variable: some_type = some_function() it is effectively a cast.

def some_function():
    return "foo"

x: int = some_function()
reveal_type(x)
test.py:5: note: Revealed type is 'builtins.int'

I think we should just allow mypy to use the return type of the initialization unless needed.

@jreback jreback added this to the 1.0 milestone Nov 17, 2019
from pandas.core.computation.common import _ensure_decoded
from pandas.core.computation.expr import BaseExprVisitor
from pandas.core.computation.ops import UndefinedVariableError, is_term

from pandas.io.formats.printing import pprint_thing, pprint_thing_encoded


class Scope(expr.Scope):
class Scope(_scope.Scope):
__slots__ = ("queryables",)
Copy link
Member Author

Choose a reason for hiding this comment

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

the medium-term goal that motivates this and the other pytables PR is that I think we can type queryables as Dict[str, Any], but it'll take a few steps to get there

@jreback jreback merged commit b9b462c into pandas-dev:master Nov 18, 2019
@jreback
Copy link
Contributor

jreback commented Nov 18, 2019

thanks

@jbrockmendel jbrockmendel deleted the cln-comp4 branch November 18, 2019 00:29
Reksbril pushed a commit to Reksbril/pandas that referenced this pull request Nov 18, 2019
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants