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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4986b31
CLN: parts of #29667
jbrockmendel Nov 17, 2019
042303c
couple i missed
jbrockmendel Nov 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pandas/core/computation/eval.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

from pandas.core.computation.engines import _engines
from pandas.core.computation.expr import Expr, _parsers, tokenize_string
from pandas.core.computation.scope import _ensure_scope
from pandas.core.computation.scope import ensure_scope

from pandas.io.formats.printing import pprint_thing

Expand Down Expand Up @@ -309,7 +309,7 @@ def eval(
_check_for_locals(expr, level, parser)

# get our (possibly passed-in) scope
env = _ensure_scope(
env = ensure_scope(
level + 1,
global_dict=global_dict,
local_dict=local_dict,
Expand Down
4 changes: 3 additions & 1 deletion pandas/core/computation/ops.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.


def __init__(self, op: str, operands, *args, **kwargs):
self.op = _bool_op_map.get(op, op)
self.operands = operands
self.encoding = kwargs.get("encoding", None)
Expand Down
7 changes: 4 additions & 3 deletions pandas/core/computation/pytables.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,18 @@

import pandas as pd
import pandas.core.common as com
from pandas.core.computation import expr, ops
from pandas.core.computation import expr, ops, scope as _scope
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


def __init__(self, level, global_dict=None, local_dict=None, queryables=None):
def __init__(self, level: int, global_dict=None, local_dict=None, queryables=None):
super().__init__(level + 1, global_dict=global_dict, local_dict=local_dict)
self.queryables = queryables or dict()

Expand All @@ -40,6 +40,7 @@ def __init__(self, name, env, side=None, encoding=None):
def _resolve_name(self):
# must be a queryables
if self.side == "left":
# Note: The behavior of __new__ ensures that self.name is a str here
if self.name not in self.env.queryables:
raise NameError("name {name!r} is not defined".format(name=self.name))
return self.name
Expand Down
10 changes: 5 additions & 5 deletions pandas/core/computation/scope.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@
from pandas.compat.chainmap import DeepChainMap


def _ensure_scope(
level, global_dict=None, local_dict=None, resolvers=(), target=None, **kwargs
):
def ensure_scope(
level: int, global_dict=None, local_dict=None, resolvers=(), target=None, **kwargs
) -> "Scope":
"""Ensure that we are grabbing the correct scope."""
return Scope(
level + 1,
Expand Down Expand Up @@ -119,7 +119,7 @@ def __init__(
self.scope.update(local_dict.scope)
if local_dict.target is not None:
self.target = local_dict.target
self.update(local_dict.level)
self._update(local_dict.level)

frame = sys._getframe(self.level)

Expand Down Expand Up @@ -251,7 +251,7 @@ def _get_vars(self, stack, scopes):
# scope after the loop
del frame

def update(self, level: int):
def _update(self, level: int):
"""
Update the current scope by going back `level` levels.

Expand Down