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

Add [all] extras to install all extra dependencies #7610

Merged
merged 4 commits into from
Apr 9, 2019

Conversation

tswast
Copy link
Contributor

@tswast tswast commented Mar 28, 2019

An [all] extra will be useful for convenience to get all the "bonus"
features like progress bars, pandas integration, and BigQuery Storage
API integration.

I also blacken the setup.py file and simplify the noxfile by using this
new [all] extra.

This is a pattern I've seen in other projects that have a lot of "extras", such as Apache Airflow (though they have a lot more the google-cloud-bigquery).
https://github.com/apache/airflow/blob/6cb735c6d301a5689ff9666349abdb6387abc7a1/setup.py#L335

@tswast tswast added the api: bigquery Issues related to the BigQuery API. label Mar 28, 2019
@tswast tswast requested review from crwilcox and a team March 28, 2019 20:50
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 28, 2019
Copy link
Contributor

@tseaver tseaver left a comment

Choose a reason for hiding this comment

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

For future reference, please avoid mixing blackening with "real" changes? The noise makes reviewing the substantive bits hard. Either split the blacken bit out into a separate commit, or else a separate PR.

else:
all_extras.extend(extras[extra])

extras["all"] = all_extras
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just normalize the extras above to always be lists?

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 fear if we miss one, then we end up with a bad dependency list.

>>> l = []
>>> l.extend("Uh, oh!")
>>> l
['U', 'h', ',', ' ', 'o', 'h', '!']

That means we still would need a check for string type or something else to validate they are all lists.

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 what I was thinking with that comment. This is why we have tests. I assume by using the [all] extra in the nox session, we'd catch such errors. I'll simplify this on Monday as you suggest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 17e65d7.

@tswast
Copy link
Contributor Author

tswast commented Mar 29, 2019

Good point re: blackening separately. Sent #7619

Copy link
Contributor Author

@tswast tswast left a comment

Choose a reason for hiding this comment

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

I've reverted the blacken changes.

else:
all_extras.extend(extras[extra])

extras["all"] = all_extras
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 fear if we miss one, then we end up with a bad dependency list.

>>> l = []
>>> l.extend("Uh, oh!")
>>> l
['U', 'h', ',', ' ', 'o', 'h', '!']

That means we still would need a check for string type or something else to validate they are all lists.

An [all] extra will be useful for convenience to get all the "bonus"
features like progress bars, pandas integration, and BigQuery Storage
API integration. It also simplifies the noxfile a bit, since the list of
extras installed in each session was getting long, especially after
using the bqstorage extra instead of in LOCAL_DEPS.
@tswast tswast requested a review from tseaver April 2, 2019 16:45
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Apr 4, 2019
@tswast tswast merged commit e8772e0 into googleapis:master Apr 9, 2019
@tswast tswast deleted the bq-all-extras branch April 9, 2019 19:12
tswast added a commit to tswast/google-cloud-python that referenced this pull request Apr 11, 2019
* Add [all] extras to install all extra dependencies

An [all] extra will be useful for convenience to get all the "bonus"
features like progress bars, pandas integration, and BigQuery Storage
API integration. It also simplifies the noxfile a bit, since the list of
extras installed in each session was getting long, especially after
using the bqstorage extra instead of in LOCAL_DEPS.

* Need fastavro to actually read rows from bqstorage.

* Blacken

* Use lists for all extras.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the BigQuery API. cla: yes This human has signed the Contributor License Agreement. 🚨 This issue needs some love.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants