Skip to content
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

Fix some ResourceWarnings. #125

Merged
merged 1 commit into from
Nov 4, 2021
Merged

Fix some ResourceWarnings. #125

merged 1 commit into from
Nov 4, 2021

Conversation

JulienPalard
Copy link
Contributor

references: #123

So the issue with #123 is funny, I just forgot to bump google-cloud-bigquery minimum version in setup.cfg.

Without bumping it to the release they introduced the context manager, it could have gone mostly right, there's tens of versions since then.

Sadly back then, they had python_requires=">=3.6" and now they have python_requires=">=3.6, <3.10", so when we install google-cloud-bigquery with Python 3.10 it forces pip to crawl back to this old version not enforcing <3.10, but not having __enter__ neither.

I enforced the right version in setup.cfg but we may not want to merge this as it makes pypinfo uninstallable with 3.10 as there does not exist a google-cloud-bigquery recent enough and compatible with py310 (the irony).

@codecov
Copy link

codecov bot commented Oct 26, 2021

Codecov Report

Merging #125 (c696b63) into master (f17e78c) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #125      +/-   ##
==========================================
+ Coverage   96.90%   96.92%   +0.01%     
==========================================
  Files           6        6              
  Lines         388      390       +2     
  Branches       47       48       +1     
==========================================
+ Hits          376      378       +2     
  Misses          4        4              
  Partials        8        8              
Impacted Files Coverage Δ
pypinfo/core.py 93.93% <100.00%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f17e78c...c696b63. Read the comment docs.

@hugovk
Copy link
Collaborator

hugovk commented Oct 27, 2021

Thanks for digging into that!

I think we should wait for google-cloud-bigquery to be compatible with 3.10 first, here's their tracking issue:

I realise we also weren't testing on 3.10 yet, it was due to other dependencies not yet supporting 3.10, but they do now. So let's also get #126 merged and included in here.

If you like, you could also split out the with open(...) change to another PR.

@hugovk
Copy link
Collaborator

hugovk commented Oct 28, 2021

Would you like to rebase/merge from master? #127 added tests to cover this.

And no rush, because I think it'll take a while for google-cloud-bigquery to release for 3.10.

@@ -37,7 +37,7 @@ python_requires = >=3.6
install_requires =
binary
click
google-cloud-bigquery >= 0.29.0
google-cloud-bigquery >= 2.11.0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

google-cloud-bigquery 2.30.0 has just been released supporting Python 3.10:

Suggested change
google-cloud-bigquery >= 2.11.0
google-cloud-bigquery >= 2.30.0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need 2.30.0:

  • In Python 3.9 we can use >=2.11.0.
  • In Python 3.10 pip will choose 2.30.0 as it have no choice.

So >= 2.11.0 looks good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could run two tests in tox, one with the minimum version, and one with the maximum version of out dependencies, to ensure it works, like I'm trying to do here: https://github.com/JulienPalard/oeis/blob/7d6c314c02983c7a47f901d07d0fffc8b772155a/tox.ini#L36;L39 ? Maybe in another PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good point that 3.10 will pick the right one. I think we'll be fine keeping it simpler and testing just a single version. Thanks!

@JulienPalard JulienPalard reopened this Nov 4, 2021
@JulienPalard
Copy link
Contributor Author

(closed/reopened to let the CI pick the new version in 3.10 so it passes)

@hugovk hugovk merged commit 745a7ae into ofek:master Nov 4, 2021
@hugovk
Copy link
Collaborator

hugovk commented Nov 4, 2021

Thank you again!

REestwick pushed a commit to REestwick/pypinfo that referenced this pull request Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants