-
-
Notifications
You must be signed in to change notification settings - Fork 152
Index: ensure Index(DatetimeIndex) returns DatetimeIndex (fixes #1440) #1456
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
Index: ensure Index(DatetimeIndex) returns DatetimeIndex (fixes #1440) #1456
Conversation
|
Hey @yusufcank34 ! Thanks for your contribution, could you add some tests to make sure your behavior is working, feel free to look into https://github.com/pandas-dev/pandas-stubs/blob/main/tests/indexes/test_datetime_index.py for examples on how to write a test (especially the check assert type structure). |
added the new test |
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.
Feel free to run poetry run poe test_all locally before you push as it mirrors the CI pipeline.
| @@ -0,0 +1,8 @@ | |||
| from typing import 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.
Since there is already a test file for datetime index it is probably cleaner to consolidate your tests there (if we made one test file by issue it would quickly become overwhelming).
Another point is that you’d need to add check(assert_type(…), pd.DatetimeIndex) in top of the assertion you have already added here.
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.
@yusufcank34 Can you remove that file since it is already covered by the other test you modified? And run pre-commit before you push again, I will merge after it.
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 @yusufcank34, nice job on your first PR!
| @@ -0,0 +1,8 @@ | |||
| from typing import 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.
@yusufcank34 Can you remove that file since it is already covered by the other test you modified? And run pre-commit before you push again, I will merge after it.
|
Thanks @yusufcank34 ! |
thk u |
Index(DatetimeIndex)erroneously givesIndex[Timestamp]#1440assert_type()to assert the type of any return value.