-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
TYP: misc typing in _libs #44349
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
TYP: misc typing in _libs #44349
Conversation
def ceil(self: _S, freq) -> _S: ... | ||
def round(self: _S, freq: str) -> _S: ... | ||
def floor(self: _S, freq: str) -> _S: ... | ||
def ceil(self: _S, freq: str) -> _S: ... |
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.
could also be BaseOffset?
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.
I added most annotations based on the documentation (when available): it mentions only strings. freq
ends up in to_offset
so it should support BaseOffset
.
def strftime(self, fmt: str) -> str: ... | ||
def to_timestamp( | ||
self, | ||
freq: str | BaseOffset | None = ..., | ||
how: str = ..., | ||
tz: Timezone | None = ..., | ||
) -> Timestamp: ... | ||
def asfreq(self, freq, how=...) -> Period: ... | ||
def asfreq(self, freq: str, how: str = ...) -> Period: ... |
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.
freq
can also be a BaseOffset
but the documentation requires a str
.
If the documentation is out of date, BaseOffset
should be added. It might be that the type was intentionally restricted to make future refactoring/simplifications possible?
I'll only comment on these cases for now.
can you rebase |
thanks @twoertwein |
Reduced the list of partially typed functions in _libs a bit.