Fix pd.concat to accept None values as input. #858
Conversation
|
You have some failures in the tests. See https://github.com/pandas-dev/pandas-stubs/actions/runs/7850999903/job/21487238109?pr=858 Be sure to set up a local environment and run tests locally. See https://github.com/pandas-dev/pandas-stubs/blob/main/docs/setup.md and https://github.com/pandas-dev/pandas-stubs/blob/main/docs/tests.md |
twoertwein
force-pushed
the
fix_concat_with_none
branch
from
2022fcc to
66df230
Compare
|
Sorry, had to force push - merging upstream messed things somehow up. |
| copy: bool = ..., | ||
| ) -> Never: ... | ||
| @overload | ||
| def concat( # type: ignore[overload-overlap] # pyright: ignore[reportOverlappingOverload] |
There was a problem hiding this comment.
overlaps because of Iterable[None]
|
|
||
| check(assert_type(pd.concat([s1, df1, df2]), pd.DataFrame), pd.DataFrame) | ||
| if TYPE_CHECKING_INVALID_USAGE: |
There was a problem hiding this comment.
@twoertwein can you explain what this block means and why this is needed? Trying to understand what was missing from my tests.
There was a problem hiding this comment.
This is testing that the type checkers see the code as invalid. Although I don't think the test is constructed correctly....
| assert_type(pd.concat({"a": None}), Never) | ||
| assert_type(pd.concat([None]), Never) |
There was a problem hiding this comment.
I think these tests should be of the form:
pd.concat({"a": None}) # type: ignore[some_mypy_error] # pyright: ignore[some_pyright_error]
pd.concat([None]) # type: ignore[some_mypy_error] # pyright: ignore[some_pyright_error]so we are checking that the type checkers see that invalid code as an error.
There was a problem hiding this comment.
I believe I tried that first - the issue is that the second call will not be checked as it cannot be reached (by the type checkers). Would need to split it into two functions.
There was a problem hiding this comment.
That makes sense. I think for consistency's sake, we should do that, although I could be convinced otherwise. If we want to use your pattern here, then add a comment to indicate why we can't just check for a specific type checker error based on your comment here.
There was a problem hiding this comment.
I think we need the assert_type only when testing at least two invalid function calls where the first one "returns" NoReturn/Never. I think we also have many cases were we do not explicitly return NoReturn/Never. In that case, we might not need the assert_type.
I would be inclined to use the assert_type only when we have to.
assert_type()to assert the type of any return value