DEPR: camelCase in offsets, get_offset by jbrockmendel · Pull Request #30340 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation23 Commits9 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
- closes API: deprecate get_offset, Offset for time rules like '30s' #4205
- tests added / passed
- passes
black pandas - passes
git diff upstream/master -u -- "*.py" | flake8 --diff - whatsnew entry
@datapythonista the good news is im able to reproduce this locally with make doc like you suggested, the bad is that I have no idea why I'm getting a bunch of these complaints:
/home/vsts/work/1/s/doc/source/reference/api/pandas.tseries.offsets.BQuarterBegin.isAnchored.rst: WARNING: document isn't included in any toctree
ideas?
BQuarterBegin.isAnchored.rst: WARNING: document isn't included in any toctree
It's because you removed those from the api doc pages, while they are still generated automatically from the class docstring. But so they also need to be included somewhere in another autosummary that adds them into the toctree. So like Marc said, we use hidden autosummaries to solve this. There should be some other examples in the api docs of that (look at the bottom of those pages).
@jorisvandenbossche my attempts at mimicing the examples in offset_frequency.rst haven't worked. in this commit i used "api.offsets.Foo.onOffset" but itt also didn't work with "Foo.onOffset", "api/Foo.onOffset", "pandas.tseries.offsets.Foo.onOffset"
@jorisvandenbossche see newest commit where i tried just about every variant I could think of, including putting them in a different file where we already use this pattern.
| @@ -61,6 +61,546 @@ public functions related to data types in pandas. |
|---|
| api/pandas.Series.from_array |
| api/pandas.Series.imag |
| api/pandas.Series.real |
| ..api/pandas.tseries.offsets.DateOffset.isAnchored |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you put it here it doesn't need the '..', see the lines above
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, didn't see what you did below :)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the actual PR, is there any difference between get_offset and to_offset that the user should be aware of? (the implementation is not exactly the same)
| return self.n == 1 |
|---|
| def onOffset(self, dt): |
| warnings.warn( |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a docstring with a ..deprecated:: directive?
| @@ -185,10 +186,29 @@ def get_offset(name: str) -> DateOffset: |
|---|
| """ |
| Return DateOffset object associated with rule name. |
| .. deprecated:: 1.0.0 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add here that to_offset should be used instead?
is there any difference between get_offset and to_offset that the user should be aware of?
I dont think get_offset is supported in a meaningful enough way for this to make sense. e.g. i just found the example in the docstring was wrong
| FutureWarning, |
|---|
| stacklevel=2, |
| ) |
| return _get_offset(name) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this call to_offset instead?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we still need _get_offset internally, just officially telling users to stay away
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, would rename maybe if you can (followon ok)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you add a deprecation note in whatsnew?
did you add a deprecation note in whatsnew?
yes
AlexKirko pushed a commit to AlexKirko/pandas that referenced this pull request
keechongtan added a commit to keechongtan/pandas that referenced this pull request
