CLN: Removed the flavor parameter in DataFrame.to_sql by gfyoung · Pull Request #13611 · 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
Conversation31 Commits1 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 }})
Deprecated in 0.14.0, so way, way overdue.
@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.
@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.
| def to_sql(self, name, con, flavor='sqlite', schema=None, if_exists='fail', |
|---|
| index=True, index_label=None, chunksize=None, dtype=None): |
| def to_sql(self, name, con, schema=None, if_exists='fail', index=True, |
| index_label=None, chunksize=None, dtype=None, *args, **kwargs): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the *args are needed here. If people were using positional kwargs, the current deprecation will not work anyways (as schema will catch the flavor)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternative is to keep flavor in the signature, but change it to None, so we can detect when people set it to 'sqlite' themselves. That solves the positional kwarg problem, but of course pollutes the signature a bit.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argh...good point. I think I'll just remove the '*args' parameter and not pollute the signature.
@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.
| @@ -492,6 +492,7 @@ Deprecations |
|---|
| - ``Categorical.reshape`` has been deprecated and will be removed in a subsequent release (:issue:`12882`) |
| - ``Series.reshape`` has been deprecated and will be removed in a subsequent release (:issue:`12882`) |
| - ``DataFrame.to_sql()`` has deprecated the ``flavor`` parameter (:issue:`13611`) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you emphasize that is is a superfluous keyword (so no reason to use it), since sqlite is the only supported connection when not using sqlalchemy?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@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.
Yes, merging! Thanks a lot
Labels
Functionality to remove in pandas
to_sql, read_sql, read_sql_query