-
-
Notifications
You must be signed in to change notification settings - Fork 144
Add check assert_type to tests for frame.py and add __new__ method for DatetimeIndex #1313
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
base: main
Are you sure you want to change the base?
Conversation
pd.DatetimeIndex(data=df["A"], tz=None, ambiguous="NaT", copy=True), | ||
|
||
check( | ||
assert_type( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the reason I added the __new__
to datetimes.pyi, prob worth putting a test in the relevant test file, please let me know if so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, actually we should probably move that test there.
Actually, looking at this function test_types_regressions()
, I think the various tests in there should be moved to the appropriate files. That function was created before we took over the repo, so splitting the tests here to the right files would be the right thing to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for doing this. Caught a few things that we should clean up as a result.
@classmethod | ||
def __new__( | ||
cls, | ||
data: AxesData | Series, | ||
freq: Frequency = ..., | ||
tz: TimeZones = ..., | ||
ambiguous: str = ..., | ||
dayfirst: bool = ..., | ||
yearfirst: bool = ..., | ||
dtype: Dtype = ..., | ||
copy: bool = ..., | ||
name: Hashable = ..., | ||
) -> Self: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just do this by making this the __init__()
method, (returning None
).
So in line 50, just modify data
to be data: AxesData | Series
# TODO the below should pass | ||
# check(assert_type(df.iat[0, 0], Scalar), np.integer) | ||
df.iat[0, 0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you fix the declaration of iat
in frame.pyi
?
# TODO the below should pass | ||
# check(assert_type(df.at[0, "col1"], Scalar), np.integer) | ||
df.at[0, "col1"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment - can you fix the declaration of at()
in frame.pyi
?
df.cumsum() | ||
df.sum(axis=0) | ||
check(assert_type(df.cumsum(), pd.DataFrame), pd.DataFrame) | ||
check(assert_type(df.sum(axis=0), pd.Series), pd.Series) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is an odd test. I wonder if it should be df.cumsum(axis=0)
, since the name of the function is test_types_cumsum()
check(assert_type(df.idxmin(), pd.Series), pd.Series) | ||
check(assert_type(df.idxmin(axis=0), pd.Series), pd.Series) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be Series[int]
. Can you fix the declaration in frame.pyi
?
check(assert_type(df.idxmax(), pd.Series), pd.Series) | ||
check(assert_type(df.idxmax(axis=0), pd.Series), pd.Series) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be Series[int]
pd.DatetimeIndex(data=df["A"], tz=None, ambiguous="NaT", copy=True), | ||
|
||
check( | ||
assert_type( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, actually we should probably move that test there.
Actually, looking at this function test_types_regressions()
, I think the various tests in there should be moved to the appropriate files. That function was created before we took over the repo, so splitting the tests here to the right files would be the right thing to do.
result: int = 10 | ||
result = df["x"].max() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can delete these 2 lines.
Cleaned up tests in tests/test_frame.py as there was no check assert_type in many places.
Added a new method for DatetimeIndex since it would not instantiate to DatetimeIndex but to Series[int].
assert_type()
to assert the type of any return value