TYP: Type hints & assert statements
State: closed

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.

Labels:
name: Error Reporting
description: Incorrect or improved errors from pandas
name: Typing
description: type annotations, mypy/pyright type checking
name: Stale

Comments:
Hello @rgkimball! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:

























There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2021-06-25 02:35:14 UTC
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...)
> 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.
> 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.

Note failing tests are due to importing Literal in <3.8
@rgkimball can you merge master (and maybe break this PR up, e.g. maybe keep type annotation additions separate from code changes)
i like the annotations added, dont think its worth futzing with the assertions for fully internal code
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.
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.