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

replace Appender decorator with doc #37384

Merged
merged 6 commits into from
Nov 11, 2020
Merged

replace Appender decorator with doc #37384

merged 6 commits into from
Nov 11, 2020

Conversation

smartvinnetou
Copy link
Contributor

@smartvinnetou smartvinnetou commented Oct 24, 2020 •
edited
Loading

@smartvinnetou smartvinnetou force-pushed the CLN-31942/replace-appender-with-doc-7 branch from 9e06dc2 to 4c42403 Compare October 25, 2020 08:47
@smartvinnetou smartvinnetou marked this pull request as ready for review October 25, 2020 09:24
arw2019
arw2019 approved these changes Oct 29, 2020
View reviewed changes
Copy link
Member

@arw2019 arw2019 left a comment

Choose a reason for hiding this comment

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

LGTM

@smartvinnetou smartvinnetou force-pushed the CLN-31942/replace-appender-with-doc-7 branch from 4c42403 to 288ed19 Compare October 29, 2020 17:27
@arw2019 arw2019 added the Docs label Oct 29, 2020
@jbrockmendel
Copy link
Member

Don't you need to Series.compare and DataFrame.compare too?

@smartvinnetou smartvinnetou force-pushed the CLN-31942/replace-appender-with-doc-7 branch 2 times, most recently from 94b6194 to c422e2c Compare November 2, 2020 20:51
@smartvinnetou
Copy link
Contributor Author

Don't you need to Series.compare and DataFrame.compare too?

Yes. Done.

I kept the use to of _shared_docs["compare"] to keep the PR small but I'd find it more intuitive to define the doc string on NDFrame.compare() and then refer to it in the calls to the doc decorator on Series.compare() and DataFrame.compare().

@smartvinnetou smartvinnetou force-pushed the CLN-31942/replace-appender-with-doc-7 branch from 38d4c54 to fef203a Compare November 5, 2020 09:10
@jbrockmendel
Copy link
Member

Yes. Done.

Have you checked locally that you get the correctly-rendered docstrings? When I try it with this PR, pandas cant even be imported.

@smartvinnetou smartvinnetou force-pushed the CLN-31942/replace-appender-with-doc-7 branch from 6899b1a to 417231d Compare November 6, 2020 18:09
@smartvinnetou
Copy link
Contributor Author

Yes. Done.

Have you checked locally that you get the correctly-rendered docstrings? When I try it with this PR, pandas cant even be imported.

You were right. The CI is passing now.

jbrockmendel
jbrockmendel approved these changes Nov 6, 2020
View reviewed changes
Copy link
Member

@jbrockmendel jbrockmendel left a comment

Choose a reason for hiding this comment

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

LGTM

@jbrockmendel jbrockmendel merged commit 90dc9ae into pandas-dev:master Nov 11, 2020
@jbrockmendel
Copy link
Member

thanks @smartvinnetou

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbrockmendel jbrockmendel jbrockmendel approved these changes

@arw2019 arw2019 arw2019 approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@smartvinnetou @jbrockmendel @arw2019 @davidmspeechmatics