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 unnecessary dependency on libcst and scripts #220

Merged
merged 8 commits into from Oct 23, 2020

Conversation

HemangChothani
Copy link
Contributor

Fixes #218

@HemangChothani HemangChothani requested review from crwilcox, busunkim96 and a team October 8, 2020 12:00
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 8, 2020
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.

#218 says:

Remove libcst as a dependency and edit the fixup script to raise an error if the library is not installed.

Please restore the scripts, and edit as directed.

@HemangChothani
Copy link
Contributor Author

@tres I found in bigquery PR , that they deleted the file and enter in exclude in syth.py so followed the same. Please guide the correct way.

@tseaver
Copy link
Contributor

tseaver commented Oct 9, 2020

@busunkim96 you specified in #218 that the fix was to remove the dependency, and update the script(s) to raise an error if libcst could not be imported. Please clarify.

@HemangChothani
Copy link
Contributor Author

@busunkim96 Could you please give some clarification?

@crwilcox
Copy link
Contributor

I can weigh in here also. the script has value, but libcst is only needed for the script. We shouldn't list it in our dependencies. Instead, we should import it, catch the import error, and end the script with a message explaining the user should python -m pip install "libcst >= 0.2.5"

@tswast was the script not useful for bigquery by chance?

@tswast
Copy link
Contributor

tswast commented Oct 20, 2020

was the script not useful for bigquery by chance?

Correct. It was useless for BigQuery since we wrap the client objects in the BigQuery Storage API and in the core library we don't use the client objects at all. We only need the generator to make the compiled protos in the core library.

Still, I think it makes sense to make libcst an optional dependency, as it conflicts with the awssdk library due to incompatible YAML version requirements.

scripts/fixup_admin_v1_keywords.py Outdated Show resolved Hide resolved
scripts/fixup_firestore_admin_v1_keywords.py Outdated Show resolved Hide resolved
scripts/fixup_firestore_admin_v1_keywords.py Outdated Show resolved Hide resolved
scripts/fixup_firestore_v1_keywords.py Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
synth.py Outdated Show resolved Hide resolved
synth.py Outdated Show resolved Hide resolved
@tseaver tseaver merged commit cd358db into googleapis:master Oct 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove dependency on libcst
4 participants