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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLN: Removed the flavor parameter in DataFrame.to_sql #13611

Merged
merged 1 commit into from Jul 19, 2016

Conversation

Projects
None yet
3 participants
@gfyoung
Member

gfyoung commented Jul 11, 2016

Deprecated in 0.14.0, so way, way overdue.

@jorisvandenbossche

This comment has been minimized.

Member

jorisvandenbossche commented Jul 11, 2016

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

@gfyoung

This comment has been minimized.

Member

gfyoung commented Jul 11, 2016

@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).

@jorisvandenbossche

This comment has been minimized.

Member

jorisvandenbossche commented Jul 11, 2016

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.

@gfyoung

This comment has been minimized.

Member

gfyoung commented Jul 11, 2016

@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

This comment has been minimized.

Member

jorisvandenbossche commented Jul 11, 2016

@gfyoung That sounds good!

@gfyoung

This comment has been minimized.

Member

gfyoung commented Jul 12, 2016

@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.

@codecov-io

This comment has been minimized.

codecov-io commented Jul 12, 2016

Current coverage is 84.37%

No coverage report found for master at 4c9ae94.

Powered by Codecov. Last updated by 4c9ae94...74e8975

@jorisvandenbossche

View changes

pandas/core/generic.py Outdated
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):

This comment has been minimized.

@jorisvandenbossche

jorisvandenbossche Jul 12, 2016

Member

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)

This comment has been minimized.

@jorisvandenbossche

jorisvandenbossche Jul 12, 2016

Member

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.

This comment has been minimized.

@gfyoung

gfyoung Jul 12, 2016

Member

Argh...good point. I think I'll just remove the '*args' parameter and not pollute the signature.

@gfyoung

This comment has been minimized.

Member

gfyoung commented Jul 14, 2016

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

@jorisvandenbossche

This comment has been minimized.

Member

jorisvandenbossche commented Jul 14, 2016

@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.

@gfyoung

This comment has been minimized.

Member

gfyoung commented Jul 14, 2016

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.

@jorisvandenbossche

This comment has been minimized.

Member

jorisvandenbossche commented Jul 14, 2016

@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

@gfyoung

This comment has been minimized.

Member

gfyoung commented Jul 15, 2016

@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.

@gfyoung gfyoung referenced this pull request Jul 15, 2016

Open

DEPR: deprecations from prior versions #6581

0 of 83 tasks complete
@gfyoung

This comment has been minimized.

Member

gfyoung commented Jul 15, 2016

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

@jorisvandenbossche

View changes

doc/source/whatsnew/v0.19.0.txt Outdated
@@ -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`)

This comment has been minimized.

@jorisvandenbossche

jorisvandenbossche Jul 15, 2016

Member

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?

This comment has been minimized.

@gfyoung

gfyoung Jul 16, 2016

Member

Done.

@jorisvandenbossche

View changes

pandas/core/generic.py Outdated
'mysql' is deprecated and will be removed in future versions, but
it will be further supported through SQLAlchemy engines.
flavor : 'sqlite', default None
DEPRECATED: this parameter will be removed in a future version

This comment has been minimized.

@jorisvandenbossche

jorisvandenbossche Jul 15, 2016

Member

similar comment here, maybe add "because only 'sqlite' is supported"

This comment has been minimized.

@gfyoung

gfyoung Jul 16, 2016

Member

Done.

@jorisvandenbossche

View changes

pandas/io/sql.py Outdated
if flavor is not None:
if flavor == 'sqlite':
warnings.warn("the 'flavor' parameter is deprecated "
"and will be removed in a future version.",

This comment has been minimized.

@jorisvandenbossche

jorisvandenbossche Jul 15, 2016

Member

and the same here. Something like: "it is not needed to specify this, because 'sqlite' is the default and only supported flavor when not using sqlalchemy" (so users don't think using sqlite itself is deprecated)

This comment has been minimized.

@gfyoung

gfyoung Jul 16, 2016

Member

Done.

"""
con = 1234 # don't need real connection for this
funcs = ['SQLiteDatabase', 'pandasSQL_builder']

This comment has been minimized.

@jorisvandenbossche

jorisvandenbossche Jul 15, 2016

Member

I would rather test the top-level DataFrame.to_sql (and then check_stacklevel can be set to True)

This comment has been minimized.

@jorisvandenbossche

jorisvandenbossche Jul 15, 2016

Member

although SQLiteDataBase can also be tested, but pandasSQL_builder is not needed I think

This comment has been minimized.

@gfyoung

gfyoung Jul 15, 2016

Member

to_sql isn't the only function that calls pandasSQL_builder that I changed though - seems more reasonable to test the function underlying all the ones whose signatures I changed IMO.

Also, why is pandasSQL_builder not needed? It has no underscore prefix --> not an internal function technically.

This comment has been minimized.

@jorisvandenbossche

jorisvandenbossche Jul 15, 2016

Member

Also, why is pandasSQL_builder not needed? It has no underscore prefix --> not an internal function technical

But is should actually have one :-)

But it' true that it is used by more than to_sql, so OK leave it like that!

This comment has been minimized.

@gfyoung

gfyoung Jul 16, 2016

Member

@jorisvandenbossche : Ah, then I'll put up a PR to underscore it then.

@jorisvandenbossche

This comment has been minimized.

Member

jorisvandenbossche commented Jul 15, 2016

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

@gfyoung

This comment has been minimized.

Member

gfyoung commented Jul 16, 2016

@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.

@gfyoung

This comment has been minimized.

Member

gfyoung commented Jul 19, 2016

@jorisvandenbossche : any updates on this?

@jorisvandenbossche

This comment has been minimized.

Member

jorisvandenbossche commented Jul 19, 2016

Yes, merging! Thanks a lot

@jorisvandenbossche jorisvandenbossche merged commit 8acfad3 into pandas-dev:master Jul 19, 2016

@gfyoung gfyoung deleted the forking-repos:to-sql-flavor-remove branch Jul 20, 2016

@jreback jreback referenced this pull request Jul 24, 2016

Open

DEPR: deprecations log for removed issues #13777

115 of 115 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment