TYP: Type hints & assert statements #42044

rgkimball · 2021-06-16T00:53:34Z

This PR addresses two concerns of best practices:

use of assert statements outside of test contexts; addressed by replacing with explicit value checks and exception raising
improved type hinting; several instances where this was previously missing or incorrect, notably DataFrame.merge for which the correct use of strings in the indicator argument did not match the boolean hint

Thanks for your time, happy to discuss or revise any of these as you see fit if they aren't consistent with the contribution guidelines.

pep8speaks · 2021-06-16T00:53:44Z

Hello @rgkimball! 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 2021-06-25 02:35:14 UTC

mzeitlin11 · 2021-06-16T02:17:54Z

Thanks for the pr @rgkimball! I remember there being an issue where use of asserts was discussed at length, but can't find it now. I think the basic philosophy behind most pandas asserts is that asserts should only be used to catch library bugs, not user errors. So any places where you've removed the assert and replaced with an explicit error, there should be a user-facing test case that can hit it.

Also, this pr is pretty complicated since it touches a lot of stuff - review would be a lot faster if you could break it up (for example, into typing addition for merge, typing for ..., change of asserts...)

simonjayhawkins · 2021-06-16T11:58:33Z

pandas/_typing.py

+MergeTypes = Literal['inner', 'outer', 'left', 'right', 'cross']
+ConcatTypes = Literal['inner', 'outer']


master currently supports python3.7 so Literal needs to be inside a TYPE_CHECKING block.

This will be changed shortly #41989, so maybe could wait for that for these changes

Thanks for the comment; either works for me - will all other TYPE_CHECKING blocks be removed at that point? If not, it may be more consistent to just add it here as well for compatibility.

After #41989 we can start the cleanup of the typing workarounds for Python3.7 (which we use since we don't have typing_extensions as a required dependency) so we hopefully can remove the TYPE_CHECKING that were specifically added to use Literal. (and others than were not available in the Python typing module until Python 3.8)

I see - agreed then it makes sense to just wait until then for most of these additions. Based on Jeff's comment about waiting until 1.3, would ~2-3 weeks be a good estimate?

yep. we've only just released the rc, so a couple of weeks at least.

#41989 now merged.

rgkimball · 2021-06-16T11:59:17Z

I think the basic philosophy behind most pandas asserts is that asserts should only be used to catch library bugs, not user errors. So any places where you've removed the assert and replaced with an explicit error, there should be a user-facing test case that can hit it.

Ah, understood! The distinction makes sense, in that case there are probably a few instances here where it should revert to the assert for protected library functions - I'll add test cases for the others.

Also, this pr is pretty complicated since it touches a lot of stuff - review would be a lot faster if you could break it up (for example, into typing addition for merge, typing for ..., change of asserts...)

No problem! I wasn't sure if it would be easier by feature or by nature of the change - would you prefer to see those as separate commits on this PR or separate PR's altogether? The latter seems like it could clutter the queue a bit but happy to accommodate what's easiest for you & the team.

simonjayhawkins · 2021-06-16T12:03:16Z

Thanks for the pr @rgkimball! I remember there being an issue where use of asserts was discussed at length, but can't find it now.

It feels like it gets discussed regularly in different PRs. we did have #32785 to try and keep those discussions in one place.

pandas/core/arrays/_ranges.py

pandas/core/apply.py

pandas/core/arrays/datetimes.py

jbrockmendel · 2021-06-21T23:44:46Z

pandas/core/frame.py

@@ -9148,7 +9148,7 @@ def merge(
        sort: bool = False,
        suffixes: Suffixes = ("_x", "_y"),
        copy: bool = True,
-        indicator: bool = False,
+        indicator: bool | str = False,


sure about this?

Per _MergeOperation you can optionally supply a string as the column name, otherwise the indicator is given a default name:

pandas/pandas/core/reshape/merge.py

Lines 655 to 658 in f68092a

if isinstance(self.indicator, str):

self.indicator_name = self.indicator

elif isinstance(self.indicator, bool):

self.indicator_name = "_merge" if self.indicator else None

Maybe I missed this, but is there a "valid column name" definition that would be more specific than str?

rgkimball · 2021-06-22T14:54:22Z

Note failing tests are due to importing Literal in <3.8

simonjayhawkins · 2021-07-07T11:32:42Z

@rgkimball can you merge master (and maybe break this PR up, e.g. maybe keep type annotation additions separate from code changes)

jbrockmendel · 2021-07-26T19:04:46Z

pandas/core/indexes/base.py

@@ -5691,7 +5691,9 @@ def _validate_indexer(self, form: str_t, key, kind: str_t):
        if key is not None and not is_integer(key):
            raise self._invalid_indexer(form, key)

-    def _maybe_cast_slice_bound(self, label, side: str_t, kind=no_default):
+    def _maybe_cast_slice_bound(
+            self, label, side: str_t, kind: Literal["loc", "getitem"] = no_default


i think needs to be amended to include no_default?

jbrockmendel · 2021-07-26T19:05:50Z

i like the annotations added, dont think its worth futzing with the assertions for fully internal code

github-actions · 2021-08-26T00:03:00Z

This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this.

mroeschke · 2021-09-02T00:55:41Z

Thanks for the PR, but it appears to have gone stale. Let us know if you're still interested in working on this and we can reopen. Closing.

simonjayhawkins reviewed Jun 16, 2021

View reviewed changes

simonjayhawkins added the Typing type annotations, mypy/pyright type checking label Jun 16, 2021

simonjayhawkins reviewed Jun 16, 2021

View reviewed changes

pandas/core/arrays/_ranges.py Outdated Show resolved Hide resolved

rgkimball changed the title ~~Type hints & assert statements~~ TYP: Type hints & assert statements Jun 16, 2021

jbrockmendel reviewed Jun 21, 2021

View reviewed changes

pandas/core/apply.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Jun 21, 2021

View reviewed changes

pandas/core/arrays/datetimes.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Jun 21, 2021

View reviewed changes

rgkimball added 5 commits June 22, 2021 08:38

Replace assert statements with argument validation

952283c

rebase to master

6a3efb9

Fix line length guidance for value check error messages

bca799d

rebase to master

1270474

Adds None default for dtype argument

ae6f764

rgkimball force-pushed the type_hinting branch from f68092a to ae6f764 Compare June 22, 2021 12:42

rgkimball added 2 commits June 22, 2021 10:32

Move Literal import out of type checking block

346210f

Addresses flake8 issues

d051c48

rgkimball added 2 commits June 22, 2021 18:57

Merge branch 'pandas-dev:master' into type_hinting

4e51736

Merge branch 'pandas-dev:master' into type_hinting

2a9cb77

simonjayhawkins added the Error Reporting Incorrect or improved errors from pandas label Jul 7, 2021

jbrockmendel reviewed Jul 26, 2021

View reviewed changes

github-actions bot added the Stale label Aug 26, 2021

mroeschke closed this Sep 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TYP: Type hints & assert statements #42044

TYP: Type hints & assert statements #42044

rgkimball commented Jun 16, 2021

pep8speaks commented Jun 16, 2021 •

edited

Loading

mzeitlin11 commented Jun 16, 2021

simonjayhawkins Jun 16, 2021

rgkimball Jun 16, 2021

simonjayhawkins Jun 16, 2021

rgkimball Jun 16, 2021

simonjayhawkins Jun 16, 2021

simonjayhawkins Jul 5, 2021

rgkimball commented Jun 16, 2021

simonjayhawkins commented Jun 16, 2021

jbrockmendel Jun 21, 2021

rgkimball Jun 22, 2021

rgkimball commented Jun 22, 2021

simonjayhawkins commented Jul 7, 2021

jbrockmendel Jul 26, 2021

jbrockmendel commented Jul 26, 2021

github-actions bot commented Aug 26, 2021

mroeschke commented Sep 2, 2021

		MergeTypes = Literal['inner', 'outer', 'left', 'right', 'cross']
		ConcatTypes = Literal['inner', 'outer']

	if isinstance(self.indicator, str):
	self.indicator_name = self.indicator
	elif isinstance(self.indicator, bool):
	self.indicator_name = "_merge" if self.indicator else None

TYP: Type hints & assert statements #42044

TYP: Type hints & assert statements #42044

Conversation

rgkimball commented Jun 16, 2021

pep8speaks commented Jun 16, 2021 • edited Loading

Comment last updated at 2021-06-25 02:35:14 UTC

mzeitlin11 commented Jun 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgkimball commented Jun 16, 2021

simonjayhawkins commented Jun 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgkimball commented Jun 22, 2021

simonjayhawkins commented Jul 7, 2021

Choose a reason for hiding this comment

jbrockmendel commented Jul 26, 2021

github-actions bot commented Aug 26, 2021

mroeschke commented Sep 2, 2021

pep8speaks commented Jun 16, 2021 •

edited

Loading