Skip to content

add type hint for core/computation/eval #26992

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

Closed
wants to merge 9 commits into from
29 changes: 20 additions & 9 deletions pandas/core/computation/eval.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,21 @@
"""

import tokenize
from typing import Any, Iterable, Union
import warnings

import numpy as np

from pandas.util._validators import validate_bool_kwarg

import pandas as pd
from pandas.core.computation.engines import _engines
from pandas.core.computation.scope import _ensure_scope

from pandas.io.formats.printing import pprint_thing


def _check_engine(engine):
def _check_engine(engine: str) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

engine needs to be Optional[str] since None is valid here (cause of one CI failure)

"""
Make sure a valid engine is passed.

Expand Down Expand Up @@ -60,7 +64,7 @@ def _check_engine(engine):
return engine


def _check_parser(parser):
def _check_parser(parser: str) -> None:
"""
Make sure a valid parser is passed.

Expand All @@ -80,7 +84,7 @@ def _check_parser(parser):
' {valid}'.format(parser=parser, valid=_parsers.keys()))


def _check_resolvers(resolvers):
def _check_resolvers(resolvers: Iterable) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

If it's possible we would want to specify the type of object within container classes, so Iterable[<type>] if it can be inferred here

if resolvers is not None:
for resolver in resolvers:
if not hasattr(resolver, '__getitem__'):
Expand All @@ -89,7 +93,7 @@ def _check_resolvers(resolvers):
'the __getitem__ method'.format(name=name))


def _check_expression(expr):
def _check_expression(expr: Any) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Kind of vague from the docstring but looks like the type here should be str (Any should be avoided as it doesn't really offer much in the way of type checking)

"""
Make sure an expression is not an empty string

Expand All @@ -107,7 +111,7 @@ def _check_expression(expr):
raise ValueError("expr cannot be an empty string")


def _convert_expression(expr):
def _convert_expression(expr: Any) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

I think also string here as well (inferring from actual usage rather than docstring)

"""
Convert an object to an expression.

Expand Down Expand Up @@ -136,7 +140,7 @@ def _convert_expression(expr):
return s


def _check_for_locals(expr, stack_level, parser):
def _check_for_locals(expr: Any, stack_level: int, parser: str) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Same here

from pandas.core.computation.expr import tokenize_string

at_top_of_stack = stack_level == 0
Expand All @@ -156,9 +160,16 @@ def _check_for_locals(expr, stack_level, parser):
raise SyntaxError(msg)


def eval(expr, parser='pandas', engine=None, truediv=True,
local_dict=None, global_dict=None, resolvers=(), level=0,
target=None, inplace=False):
def eval(expr: str,
parser: str = 'pandas',
engine: str = None,
truediv: bool = True,
local_dict: dict = None,
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using dict can you use the generic Dict from the typing module and add subscripts for the key / value types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this feedback. I looked through the where local_dict be used https://github.com/pandas-dev/pandas/blob/master/pandas/core/computation/scope.py#L103, but I cannot find out what kind of dict is this? [str, str] ? Thanks !

Copy link
Member

Choose a reason for hiding this comment

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

Hmm I'm not intimately familiar with this code but from what I gather should probably be Dict[str, Any] to simulate what can be injected into locals() and/or globals()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I will update soon !

global_dict: dict = None,
resolvers: Iterable = (),
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs to be List[Dict] per documentation (may also help CI failure)

level: int = 0,
target: Union[np.ndarry, pd.DataFrame, pd.Series] = None,
Copy link
Member

Choose a reason for hiding this comment

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

Typo on ndarray but this can also return a scalar right?

inplace: bool = False) -> Union[np.ndarry, pd.DataFrame, pd.Series]:
"""
Evaluate a Python expression as a string using various backends.

Expand Down