4304r if_exists == 'fail' should actually work by hayd · Pull Request #6164 · 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
Conversation11 Commits2 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 }})
The if_exists argument in io.sql.write_frame needed data validation because the logic of the function implicitly used 'append' if the argument value was any string that was not either 'fail' or 'replace'. I added a new unit test to support the requirement.
BUG: Fix if_exists='replace' functionality in io.sql.write_frame
This should resolve issues pandas-dev#2971 and pandas-dev#4110
CLN: Refactor in between test clean ups to be more DRY
TST: Complete test coverage for if_exists uses in io.sql.write_frame
CLN: Refactor to make interaction between exists and if_exists clearer
This refactor results in the function logic being clearer, since if_exists is only relevant when exists is True, the program flow is better served to have if_exists control flow only when exists is True
BUG: Fix regression introduced by c28f11a0041a9f3b25f33b0539e42fa802b1d8d4
sqlite3 convenience function executescript not available in other database flavors.
TST: Adding if_exist test for mysql flavor
Did you review the code for sanity?
I haven't looked at the SQL code. ever.
jreback added a commit that referenced this pull request
4304r if_exists == 'fail' should actually work
@hayd looks good....(and nice that you added some more tests).
Must take care to not drop this when merging sql stuff in 0.14.
Need to test this with the new sql branch. Will come up when bring back the old sql test file... (which atm I can't get to pass...)
@hayd Can you take this on? To resurrect the old test suite?
@jorisvandenbossche @hayd I thought I fixed this with the new code? Pretty sure it's in the new tests too - it should test each allowed value of if_exists. Should also raise ValueError if invalid if_exists arg passed, though could probably do with more robust checking in code and tests
@mangecoeur It is indeed already tested in the new tests. But that doesn't mean that we shouldn't try if the old tests still pass?
@jorisvandenbossche I got the impression on reviewing the legacy tests that they were a bit of a mess, including testing undocumented APIs such as the "retry" option. That was why i ported the coverage to the new test architecture rather than extending the legacy ones - i tried to keep the test concepts rather than the test code.
OK, fine with me. I think @hayd is more familiar with the previous code base/tests, so maybe he can give a final tought on that.
20 tasks