ENH: update pandas-gbq to 0.8.0, adds credentials arg by tswast · Pull Request #23662 · 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
Conversation16 Commits3 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 }})
- N/A closes #xxxx
- tests added / passed
- passes
git diff upstream/master -u -- "*.py" | flake8 --diff - whatsnew entry
| reauth=False, if_exists='fail', private_key=None, |
|---|
| auth_local_webserver=False, table_schema=None, location=None, |
| progress_bar=True, verbose=None): |
| reauth=False, if_exists='fail', auth_local_webserver=False, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize private_key was listed as a breaking change for pandas-gbq but don't believe it was ever deprecated on our end. Is there no other way to go about this than just removing?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops sorry I see it was just moved around in the signature. Any reason for doing that? Seems like it would be safer to keep in it's current position
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved it to the end because that's where I was told to move verbose when we deprecated that. Seeing as there are already many keyword arguments preceding it, I don't think there's a risk of people using the position of the argument.
| @@ -19,7 +19,7 @@ |
|---|
| api_exceptions = pytest.importorskip("google.api_core.exceptions") |
| bigquery = pytest.importorskip("google.cloud.bigquery") |
| service_account = pytest.importorskip("google.oauth2.service_account") |
| pandas_gbq = pytest.importorskip('pandas_gbq') |
| pandas_gbq = pytest.importorskip("pandas_gbq", minversion="0.8.0") |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no
you need to support the stated min. version
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not bump the minimum version?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropped. Now it'll error instead of skip if an old version of pandas-gbq is installed.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm where is this actually erroring?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'll import at the call to pandas_gbq.read_gbq() or pandas_gbq.to_gbq() with a "unexpected 'credentials' argument". There are a couple of versions of pandas-gbq that used **kwargs in read_gbq(). Unfortunately, it will just ignore the extra argument in those older versions.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do those kwargs begin at a certain version? Wondering if we shouldn't have test coverage for those to ensure behavior. Alternately could consider bumping the min version in our requirements files
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| reflect changes from the `Pandas-GBQ library version 0.8.0 |
|---|
| https://pandas-gbq.readthedocs.io/en/latest/changelog.html#changelog-0-8-0`__. |
| Adds a ``credentials`` argument, which enables the use of any kind of |
| `google-auth credentials |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you also add this version change in the dependencies changed section (further down). and update install.rst
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thoo added a commit to thoo/pandas that referenced this pull request
tm9k1 pushed a commit to tm9k1/pandas that referenced this pull request
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request