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: remove libcst as a required dependency #389

Merged
merged 3 commits into from
Feb 14, 2022

Conversation

adamchainz
Copy link
Contributor

@adamchainz adamchainz commented Jan 26, 2022

libcst is only used for a one-off "fixup" script. It’s not necessary to always depend on it.

It caused an issue on one project as old libcst versions don't ship macOS M1 wheels, and it took time to figure this out.

(Also the instructions for using the fixup script were incorrect.)

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • [nope] Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • [I hope so] Ensure the tests and linter pass
  • [it shouldn't] Code coverage does not decrease (if any source code was changed)
  • [yes] Appropriate docs were updated (if necessary)

Fixes #395 🦕

libcst is only used for a one-off "fixup" script. It’s not necessary to always depend on it.

It caused an issue on one project as old libcst versions don't ship macOS M1 wheels, and it took time to figure this out.

(Also the instructions for using the fixup script were incorrect.)
@adamchainz adamchainz requested a review from a team January 26, 2022 16:34
@adamchainz adamchainz requested a review from a team as a code owner January 26, 2022 16:34
@product-auto-label product-auto-label bot added the api: bigquerystorage Issues related to the googleapis/python-bigquery-storage API. label Jan 26, 2022
@tswast
Copy link
Contributor

tswast commented Feb 14, 2022

Thanks for the fix! I agree that this dependency is not necessary in most cases.

@tswast tswast changed the title Remove libcst dependency fix: remove libcst as a required dependency Feb 14, 2022
UPGRADING.md Outdated Show resolved Hide resolved
@tswast tswast added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 14, 2022
@yoshi-kokoro yoshi-kokoro removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Feb 14, 2022
@adamchainz
Copy link
Contributor Author

Would you be able to replicate this to other google python packages? In just the one google project I work on I see many other google packages depend on libcst for similar scripts: google-cloud-kms, google-cloud-pubsub, google-cloud-secret-manager, etc.

@tswast
Copy link
Contributor

tswast commented Feb 14, 2022

Would you be able to replicate this to other google python packages? In just the one google project I work on I see many other google packages depend on libcst for similar scripts: google-cloud-kms, google-cloud-pubsub, google-cloud-secret-manager, etc.

I only maintain the BigQuery-related packages, but I'll bring this up with our core Python team.

@tswast tswast merged commit 92b503a into googleapis:main Feb 14, 2022
@buhrmann
Copy link

Btw, although this has been fixed almost a year ago, there is no version of this (>=2.12) on conda-forge still. Not sure whether you officially support conda packages, but at the moment only pypi seems to work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquerystorage Issues related to the googleapis/python-bigquery-storage API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

libcst is not actually required
4 participants