add type hint for core/computation/eval #26992
Conversation
|
I am not fully sure about target variable type and return type. If anyone could take a look and give some feedback, that will be great. Thanks |
pandas/core/computation/eval.py
Outdated
| parser: str = 'pandas', | ||
| engine: str = None, | ||
| truediv: bool = True, | ||
| local_dict: dict = None, |
There was a problem hiding this comment.
Instead of using dict can you use the generic Dict from the typing module and add subscripts for the key / value types?
There was a problem hiding this comment.
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 !
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
Thanks! I will update soon !
pandas/core/computation/eval.py
Outdated
| @@ -89,7 +93,7 @@ def _check_resolvers(resolvers): | |||
| 'the __getitem__ method'.format(name=name)) | |||
|
|
|||
|
|
|||
| def _check_expression(expr): | |||
| def _check_expression(expr: Any) -> None: | |||
There was a problem hiding this comment.
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)
pandas/core/computation/eval.py
Outdated
| @@ -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: | |||
There was a problem hiding this comment.
I think also string here as well (inferring from actual usage rather than docstring)
pandas/core/computation/eval.py
Outdated
| @@ -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: | |||
pandas/core/computation/eval.py
Outdated
| global_dict: dict = None, | ||
| resolvers: Iterable = (), | ||
| level: int = 0, | ||
| target: Union[np.ndarry, pd.DataFrame, pd.Series] = None, |
There was a problem hiding this comment.
Typo on ndarray but this can also return a scalar right?
Codecov Report
@@ Coverage Diff @@
## master #26992 +/- ##
==========================================
- Coverage 91.99% 91.99% -0.01%
==========================================
Files 180 180
Lines 50774 50777 +3
==========================================
- Hits 46711 46710 -1
- Misses 4063 4067 +4
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #26992 +/- ##
=========================================
+ Coverage 91.68% 91.99% +0.3%
=========================================
Files 182 180 -2
Lines 50246 50779 +533
=========================================
+ Hits 46069 46713 +644
+ Misses 4177 4066 -111
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Comments on 3/4 CI failures. Not sure about last one regarding metaclass but may be related to python/mypy#730 if you can take a look
pandas/core/computation/eval.py
Outdated
| 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: |
There was a problem hiding this comment.
engine needs to be Optional[str] since None is valid here (cause of one CI failure)
| @@ -136,7 +140,7 @@ def _convert_expression(expr): | |||
| return s | |||
|
|
|||
|
|
|||
| def _check_for_locals(expr, stack_level, parser): | |||
| def _check_for_locals(expr: str, stack_level: int, parser: str) -> None: | |||
There was a problem hiding this comment.
Can just add return None at the end of the body to appease CI
pandas/core/computation/eval.py
Outdated
| truediv: bool = True, | ||
| local_dict: Dict[str, Any] = None, | ||
| global_dict: Dict[str, Any] = None, | ||
| resolvers: Iterable = (), |
There was a problem hiding this comment.
I think this needs to be List[Dict] per documentation (may also help CI failure)
| @@ -353,3 +366,5 @@ def eval(expr, parser='pandas', engine=None, truediv=True, | |||
| # We want to exclude `inplace=None` as being False. | |||
| if inplace is False: | |||
| return target if target_modified else ret | |||
| else: | |||
| truediv: bool = True, | ||
| local_dict: List[Dict[str, Any]] = None, | ||
| global_dict: List[Dict[str, Any]] = None, | ||
| resolvers: Tuple = (), |
There was a problem hiding this comment.
So does this only accept a Tuple of Dict? If so we want to add Tuple[Dict[?, ?], ...] as the annotation (replacing ? with appropriate types) and update the docstring which currently suggests a list is appropriate
| @@ -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: | |||
There was a problem hiding this comment.
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
|
Removing from 0.25 milestone - @xcz011 if you can merge master and address comments can keep with this though |
Sorry about leaving this open, I think I will try to finish this soon. Thanks |
|
Can you merge master? Couldn't see old build failures |
|
closing as stale, if you'd like to continue pls ping to reopen |
git diff upstream/master -u -- "*.py" | flake8 --diff