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.

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

CLN 31942/replace appender with doc 3 #33277

Conversation

smartvinnetou
Copy link
Contributor

@smartvinnetou smartvinnetou commented Apr 3, 2020

@smartvinnetou smartvinnetou force-pushed the CLN-31942/replace-appender-with-doc-3 branch from e0209f5 to f6b3953 Compare April 4, 2020 08:25
@pep8speaks
Copy link

pep8speaks commented Apr 4, 2020

Hello @smartvinnetou! 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 2020-06-14 20:34:47 UTC

@smartvinnetou smartvinnetou force-pushed the CLN-31942/replace-appender-with-doc-3 branch from 4e7dded to 062a406 Compare April 9, 2020 20:10
@smartvinnetou smartvinnetou marked this pull request as ready for review April 10, 2020 07:30
@smartvinnetou smartvinnetou force-pushed the CLN-31942/replace-appender-with-doc-3 branch 2 times, most recently from 2591055 to f5409be Compare April 16, 2020 19:38
@smartvinnetou smartvinnetou force-pushed the CLN-31942/replace-appender-with-doc-3 branch 2 times, most recently from cfc1a63 to 23bb2aa Compare April 18, 2020 12:04
@WillAyd WillAyd added the Docs label Apr 22, 2020
@smartvinnetou smartvinnetou requested a review from WillAyd April 27, 2020 10:46
@smartvinnetou smartvinnetou force-pushed the CLN-31942/replace-appender-with-doc-3 branch 3 times, most recently from 2f92f60 to 10cb001 Compare May 9, 2020 07:04
@smartvinnetou
Copy link
Contributor Author

smartvinnetou commented May 9, 2020

Can someone help with reviewing and merging this PR?

@smartvinnetou smartvinnetou force-pushed the CLN-31942/replace-appender-with-doc-3 branch 3 times, most recently from 1330020 to f15188e Compare May 15, 2020 09:46
@smartvinnetou smartvinnetou force-pushed the CLN-31942/replace-appender-with-doc-3 branch 5 times, most recently from 40ae8b9 to f2bd1a2 Compare May 27, 2020 21:34
@WillAyd
Copy link
Member

WillAyd commented Jun 5, 2020

Looks like some merge conflicts in - can you fix that up?

@smartvinnetou smartvinnetou force-pushed the CLN-31942/replace-appender-with-doc-3 branch from f2bd1a2 to 164a63d Compare June 6, 2020 07:16
@smartvinnetou
Copy link
Contributor Author

Looks like some merge conflicts in - can you fix that up?

done

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

@smartvinnetou for future reference would be nice if you can make smaller PRs. Something with this many changes is much harder to review at once, so typically better to have multiple PRs with smaller changes.

That said this lgtm - @datapythonista care to look?

@smartvinnetou
Copy link
Contributor Author

@smartvinnetou for future reference would be nice if you can make smaller PRs. Something with this many changes is much harder to review at once, so typically better to have multiple PRs with smaller changes.

@WillAyd I appreciate that. That's why I stopped making more changes. I started other branches but soon found that there was a non trivial overhead on maintaining them and keeping them current. I hit a pause button until PRs start to be merged.

@datapythonista
Copy link
Member

@smartvinnetou can you please resolve the conflicts?

As said, smaller PRs are faster to review, get merged faster, and conflicts are less likely.

@smartvinnetou smartvinnetou force-pushed the CLN-31942/replace-appender-with-doc-3 branch from 164a63d to 42ebde7 Compare June 8, 2020 17:52
@smartvinnetou smartvinnetou force-pushed the CLN-31942/replace-appender-with-doc-3 branch from c5b7133 to 69f28b5 Compare June 14, 2020 20:34
@smartvinnetou
Copy link
Contributor Author

@smartvinnetou pls merge master again.

Conflict resolved.

@jorisvandenbossche @TomAugspurger @datapythonista can you give a quick look again and merge if good.

@datapythonista datapythonista merged commit 80ba4c4 into pandas-dev:master Jun 19, 2020
@datapythonista
Copy link
Member

Thanks @smartvinnetou, very nice clean up.

Once the development documentation is updated, can you please have a quick look at the updated pages, and see if everything is rendering as expected.

Thanks!

@smartvinnetou
Copy link
Contributor Author

Thanks @smartvinnetou, very nice clean up.

Once the development documentation is updated, can you please have a quick look at the updated pages, and see if everything is rendering as expected.

Thanks!

@datapythonista, where do I find this documentation?

@datapythonista
Copy link
Member

wesbarnett pushed a commit to wesbarnett/pandas that referenced this pull request Jun 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants