BUG: data written with to_sql legacy mode (sqlite/mysql) not persistent GH6846
State: closed

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


Labels:

Comments:
@Acanthostega Thanks a lot! Already looking good, just some comments. 
If you address them, you amend the commit and then push force again (`git push -f origin`), and the PR will automatically update.

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). 
And furthermore, it also makes it more difficult to review this PR (to see what is really changed).

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.

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). 
But just a last question: can you squash the two commits? Then it is good to merge!

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 :-)