4304r if_exists == 'fail' should actually work #6164
Conversation
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. |
DOC add to release notes
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. |
|
cc @mangecoeur will see this on rebasing.... |
|
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. |
fixes #4110 #4304
#4304 rebased and passes travis.
cc @y-p