BUG: data written with to_sql legacy mode (sqlite/mysql) not persistent GH6846 #6875
Conversation
| @@ -507,8 +509,71 @@ class TestSQLLegacyApi(_TestSQLApi): | |||
| """ | |||
| flavor = 'sqlite' | |||
|
|
|||
| def connect(self): | |||
| return sqlite3.connect(':memory:') | |||
| def setUp(self): | |||
There was a problem hiding this comment.
I don't think it is needed to repeat that here (the setUp function I mean). Just call self._load_test2_data in the beginning of your test test_sql_open_close
|
@Acanthostega Thanks a lot! Already looking good, just some comments. BTW, we always recommend to make a seperate branch for a PR (so you don't work on the master branch). It's OK for now, but if you would contribute more in the future, it's better to commit changes directly to master. If you have further questions, just ask! |
|
I made modifications according to comments and also added corrections in the code for pep8. There was importations of pymysql and psycopg2 not used so I removed them (maybe I'm wrong and they are useful). I have a question about branches. If I have modifications to done, I create my personal branch, make modifications, commit and push my branch into the fork (without merging to master). Then for the pull request, I specify that I want to merge my branch to the master one of pandas. Am I right or should I put it in an other branch? |
|
That's correct. |
|
@Acanthostega Sorry, we should maybe be clearer about this, but in general we don't like such massive PEP8 changes in the same commit for another change. In a PR, just make sure that the new code for the contribution or the fix follows PEP8, but for cleaning up other code we like to do this seperate and at certain moments (to keep history and git blame cleaner, and to not get too much conflicts with other PRs). So could you seperate the actual fix and tests here? But for the branch, like @cpcloud said, you are correct. Normally this is also what github will propose by default. |
| sql.has_table( | ||
| 'test_frame1', | ||
| self.conn, | ||
| flavor='sqlite' |
There was a problem hiding this comment.
This is also not really PEP8, these can stay on one line up to 80 chars.
|
I updated to only put tests without pep8 corrections. I should have read with more attention the guidelline for contribution before, since all is clear inside! Now, if tests pass, I have to update changelog file in an other commit. Am I right? |
|
Looking good! I tested the test and it works, and also Travis is green. I think adding something to the changelog is not really necessary, as this fixes something that was not yet released (there will be one big item in the changelog about all sql stuff that is improved). |
…nt GH6846 Correction for missing commit of data into SQL database in legacy code and add test to avoid future regression. Correct tests according to comments
|
I think it should be OK, except the commit message maybe... |
|
@Acanthostega Thanks a lot for your contribution! Merging now If you would want to tackle other issues on sql (see the list here #6292), let you go :-) |
BUG: data written with to_sql legacy mode (sqlite/mysql) not persistent GH6846
I added a call to commit method for pushing data into the SQL database and not only in the memory.
Tests are updated too in order to avoid future regression with a closed connection between writing and reading in the database. Since all the data in a memory based database in sqlite3 should be removed when closed, I create a temporary file used as database. The NamedTemporaryFile documentation states that it should be cross platforms but I don't have any Windows to test it. Normally doesn't break something. I never have done a pull request before, so let me know if something is bad. It's my first time and I'm happy to do it with pandas...
Closes #6846