CLN: Removed the flavor parameter in DataFrame.to_sql
State: closed

Deprecated in `0.14.0`, so way, way overdue.


Labels:
name: IO SQL
description: to_sql, read_sql, read_sql_query
name: Deprecate
description: Functionality to remove in pandas

Comments:
@gfyoung We cannot just remove the flavor keyword. It was only deprecated for 'mysql', not for 'sqlite' (wihch is a pitty ..)

@jorisvandenbossche : But the argument accepts only one value now (`sqlite`), so why not remove it?  Are we planning to add new functionality to it in the future?  Judging from the documentation, I did not get that impression, as we seem to be relegating more of the functionality to `SQLAlchemy` (it doesn't even respect the parameter if `SQLAlchemy` is available).

No, we are certainly not planning that. And in retrospect, it's a pitty that we didn't deprecate the full flavor keyword. But we have to deal with it now. I think we should give a nice error/warning message in case of `flavor='sqlite'`. So either deprecated that, or 'remove' it but still capture it's usage to display a more informative error message.

@jorisvandenbossche : How does adding a `**kwargs` parameter sound?  Then we can still accept it and warn about it if the user passes it in.

@gfyoung That sounds good!

@jorisvandenbossche : added `*args` and `**kwargs` along with a validation function AND skipped all of the test suite that relied on the `flavor="mysql"` connection.  However, I would like to add a test afterwards to make sure the `flavor` parameter is properly deprecated without these functions getting abused, but I just want to make sure these initial changes appeal the Travis.

## [Current coverage](https://codecov.io/gh/pydata/pandas/pull/13611?src=pr) is **84.37%**

> No coverage report found for **master** at 4c9ae94.
> 
> Powered by [Codecov](https://codecov.io?src=pr). Last updated by [4c9ae94...74e8975](https://codecov.io/gh/pydata/pandas/compare/4c9ae94f1ee4d867e2d92735d5755d43daef618d...74e8975e6ff670f422c966fc1d53f38e8ba4a297?src=pr)

@jreback , @jorisvandenbossche : Updated the signatures, and Travis is still giving the green light.  Ready to merge if there are no other concerns.

@gfyoung yeah, I saw your ping, but I also have other work :-)

I worry a bit about the usage of `flavor` as positional arg .. We actually used it like at some places in the tests I saw in the diff. But it is difficult to judge whether this is used like that by users (a quick stackoverflow search didn't turn anything up). 
It would be the safer option to leave it in the signature I think until we actually remove it.

Yeah...that is a fair point.  Original purpose was to just remove the 'mysql' option, so let me scale this PR back down to that.

@gfyoung Yes, but I think it would still be good to deprecated the flavor arg, so we can remove it later on. So if you want to do that, I don't it is a large change from what you have now, it's just adding it back in the signature and replacing the kwarg validator with only the deprecation part of it

@jorisvandenbossche : Updated the signature and put the `flavor` parameter back.  However, it was tricky to issue warnings because `flavor` is a keyword with default value as `"sqlite"` in MOST cases.  As we don't want to punish users who don't pass it in, and since it has no effect anymore, I changed the default value for `flavor` in ALL signatures to `None`.  Travis is happy with this change, so ready to merge if there are no other concerns.

@jreback, @jorisvandenbossche : rebased and Travis is still happy - ready to merge if there are no other concerns.

@gfyoung Looks good! (using flavor=None is perfect for detecting if user has specified it themselves) Few minor comments.

@jorisvandenbossche : made the requested doc changes (no need to run tests, hence the `[ci skip]`, so ready to merge if there are no other concerns.

@jorisvandenbossche : any updates on this?

Yes, merging! Thanks a lot