Skip to content
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.

Sign up for GitHub

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

Jump to bottom

DOC: append deprecation #45587

Merged
merged 10 commits into from
Feb 11, 2022
Merged

DOC: append deprecation #45587

merged 10 commits into from
Feb 11, 2022

Conversation

gesoos
Copy link
Contributor

@gesoos gesoos commented Jan 24, 2022

MarcoGorelli
MarcoGorelli requested changes Jan 24, 2022
View reviewed changes
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gesoos

Should the suggested alternative be pd.concat instead ofDataFrame/Series.concat?

@gesoos
Copy link
Contributor Author

gesoos commented Jan 25, 2022

Sorry, hurried too much, changed it to pandas.concat and especially added it to the Series.append method too

@jreback jreback added this to the 1.4.1 milestone Jan 26, 2022
jreback
jreback reviewed Jan 26, 2022
View reviewed changes
pandas/core/frame.py Outdated
@@ -8936,6 +8936,10 @@ def append(

Columns in `other` that are not in the caller are added as new columns.

.. deprecated:: 1.4.0
append is deprecated,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if we need 'append is deprecated'. This IS the append doc-string. We don't do this anywhere else.

Also capital the first letter of the sentence.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jreback isn't it the same as in lookup https://pandas.pydata.org/docs/reference/api/pandas.DataFrame.lookup.html , where it says

DataFrame.lookup is deprecated

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took inspiration from existing messages that describe the issue. Since append refers to the method, I would not capitalize it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure lookup is a full message, this is not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am ok if its a full sentence in a similar way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about https://pandas.pydata.org/docs/reference/api/pandas.DataFrame.slice_shift.html?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gesoos I think if you make it similar to how lookup is (suggest to use concat instead, and point to the relevant part of the user guide if there is one - check the whatsnew note) it'll be OK

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah we would like to make these consistent.

@jreback jreback changed the title DOC GH44539 DOC: append deprecation Jan 26, 2022
@jreback
Copy link
Contributor

jreback commented Jan 30, 2022

@gesoos if you can update

@gesoos
Copy link
Contributor Author

gesoos commented Jan 31, 2022

@gesoos if you can update

Updated it, tried linking to the deprecation note in the what's new, but I am not 100% sure the formatting is correct, so let me know and I will fix it

@jreback
Copy link
Contributor

jreback commented Jan 31, 2022

@gesoos its not necessasry to add that link, not what i meant.

its failing: https://github.com/pandas-dev/pandas/runs/5012248628?check_suite_focus=true

just make this simple.

@pep8speaks
Copy link

pep8speaks commented Feb 1, 2022 •
edited
Loading

Hello @gesoos! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-02-11 12:24:25 UTC

jreback
jreback requested changes Feb 1, 2022
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you need to build the doc page, this is still failing.

@MarcoGorelli
Copy link
Member

@gesoos check https://pandas.pydata.org/docs/development/contributing_documentation.html#building-the-documentation and the examples using --single

@gesoos
Copy link
Contributor Author

gesoos commented Feb 2, 2022

Sorry, this will take me a few more days, I need a new computer and it will only be available tomorrow. Thanks for the hint with the documentation building, was not aware that it's possible!

@jreback
Copy link
Contributor

jreback commented Feb 9, 2022

@gesoos if you can update

@simonjayhawkins
Copy link
Member

I think the deprecation was adequately documented in the release notes https://pandas.pydata.org/pandas-docs/dev/whatsnew/v1.4.0.html#deprecated-frame-append-and-series-append and all other documentation was updated appropriately in #44539. The only thing missing is the deprecated tag for the the method itself?

having .. deprecated:: 1.4.0 should be sufficient as users can refer to the release notes for 1.4?

@MarcoGorelli
Copy link
Member

I think the deprecation was adequately documented in the release notes https://pandas.pydata.org/pandas-docs/dev/whatsnew/v1.4.0.html#deprecated-frame-append-and-series-append and all other documentation was updated appropriately in #44539. The only thing missing is the deprecated tag for the the method itself?

having .. deprecated:: 1.4.0 should be sufficient as users can refer to the release notes for 1.4?

sounds good - @gesoos want to update it to this?

@gesoos
Copy link
Contributor Author

gesoos commented Feb 9, 2022

@MarcoGorelli I can update it, but I cannot build the documentation. If you have a more detailed description on how this should work than what can be found here, please let me know... There is a known issue with the Dockerfile, which I already noticed and am using the suggested fix, but it throws ModuleNotFound at every corner, for:

  • docutils
  • numpy
  • pytz
  • dateutil
  • pandas._libs.interval (which is where I gave up)

@MarcoGorelli
Copy link
Member

Hey - I'd suggest following the conda instructions

@rendner rendner mentioned this pull request Feb 9, 2022
Merged
1 task
@jreback
Copy link
Contributor

jreback commented Feb 9, 2022

we should do this for 1.4.1

@simonjayhawkins simonjayhawkins added the Blocker Blocking issue or pull request for an upcoming release label Feb 9, 2022
@simonjayhawkins
Copy link
Member

sorry @gesoos to mess you around. This was covered in our monthly dev meeting and agreed that a one-liner would be helpful

have updated this PR as release is planned for tomorrow.

image

simonjayhawkins
simonjayhawkins reviewed Feb 11, 2022
View reviewed changes
pandas/core/frame.py Outdated
@@ -9031,17 +9034,18 @@ def append(
Returns
-------
DataFrame
A new DataFrame consisting of the rows of caller and the rows of `other`.
A new DataFrame consisting of the rows of caller and the rows of
`other`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

urgh. this was auto re-wrapped. Can revert for clarity or leave as is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine as long as renders ok

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm seems like the rewarpping didn't work anyway

flake8..........................................................................................................Failed
- hook id: flake8
- exit code: 1

pandas/core/frame.py:9017:89: E501 line too long (116 > 88 characters)
pandas/core/series.py:2884:89: E501 line too long (116 > 88 characters)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine as long as renders ok

yeah it rendered ok, but have reverted anyway

@jreback
Copy link
Contributor

jreback commented Feb 11, 2022

merge when ready

@simonjayhawkins
Copy link
Member

merge when ready

am just rebuilding docs after flake8 fixup to be sure.

@simonjayhawkins simonjayhawkins merged commit 70e4c5b into pandas-dev:main Feb 11, 2022
@simonjayhawkins
Copy link
Member

Thanks @gesoos

@simonjayhawkins simonjayhawkins removed the Blocker Blocking issue or pull request for an upcoming release label Feb 11, 2022
@meeseeksmachine meeseeksmachine mentioned this pull request Feb 11, 2022
Merged
meeseeksmachine pushed a commit to meeseeksmachine/pandas that referenced this pull request Feb 11, 2022
@gesoos @meeseeksmachine
Backport PR pandas-dev#45587: DOC: append deprecation
9a729e6
jreback pushed a commit that referenced this pull request Feb 11, 2022
@meeseeksmachine @gesoos
Backport PR #45587: DOC: append deprecation (#45942)
47e3b40 
Co-authored-by: gesoos <66533786+gesoos@users.noreply.github.com>
phofl pushed a commit to phofl/pandas that referenced this pull request Feb 14, 2022
@gesoos @phofl
DOC: append deprecation (pandas-dev#45587)
2fabac7
yehoshuadimarsky pushed a commit to yehoshuadimarsky/pandas that referenced this pull request Jul 13, 2022
@gesoos @yehoshuadimarsky
DOC: append deprecation (pandas-dev#45587)
4ed0728
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonjayhawkins simonjayhawkins simonjayhawkins left review comments

@jreback jreback jreback approved these changes

@MarcoGorelli MarcoGorelli Awaiting requested review from MarcoGorelli

Assignees
No one assigned
Labels
Docs
Projects
None yet
Milestone
1.4.1
Development

Successfully merging this pull request may close these issues.
5 participants
@gesoos @jreback @pep8speaks @MarcoGorelli @simonjayhawkins