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

Eliminated _WriterBase class, removed unused fixtures from methods in pandas/io/excel/test_writers.py #28753

Merged

Conversation

dayoreke
Copy link
Contributor

@dayoreke dayoreke commented Oct 2, 2019

@dayoreke dayoreke changed the title Eliminated _WriterBase class, removed unused fixtures from methods. Eliminated _WriterBase class, removed unused fixtures from methods in pandas/io/excel/test_writers.py Oct 2, 2019
@simonjayhawkins simonjayhawkins added Clean IO Excel read_excel, to_excel Testing pandas testing functions or related to the test suite labels Oct 2, 2019
@gfyoung gfyoung requested a review from WillAyd October 2, 2019 21:42
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.

Thanks for taking a look - this one might be tricky. Do you see a good way to keep all of the fixtures together? The downside to this approach is its not very clear the order in which the fixtures and parameters are getting injected

@pep8speaks
Copy link

pep8speaks commented Oct 3, 2019

Hello @dayoreke! 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 2019-10-13 21:31:37 UTC

Copy link
Member

@simonjayhawkins simonjayhawkins left a comment

Choose a reason for hiding this comment

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

@simonjayhawkins
Copy link
Member

Thanks for taking a look - this one might be tricky.

this addresses Eliminate _WriterBase class and engine and ext injected into tests #26662 (comment) from #26784

could split into two PRs if it makes it easier to review.

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.

OK I am on board with this ex @simonjayhawkins outstanding comments

@dayoreke dayoreke force-pushed the eliminate_writer_base_class branch from f1dd871 to f2db75d Compare October 13, 2019 20:39
@simonjayhawkins simonjayhawkins added this to the 1.0 milestone Oct 13, 2019
@simonjayhawkins
Copy link
Member

test_basics_with_nan failing.

Copy link
Member

@simonjayhawkins simonjayhawkins left a comment

Choose a reason for hiding this comment

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

@dayoreke lgtm. ping on green.

@simonjayhawkins simonjayhawkins merged commit 06a6b49 into pandas-dev:master Oct 13, 2019
@simonjayhawkins
Copy link
Member

Thanks @dayoreke Very nice!

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
bongolegend pushed a commit to bongolegend/pandas that referenced this pull request Jan 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean IO Excel read_excel, to_excel Testing pandas testing functions or related to the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants