Skip to content

Conversation

@gautamomento
Copy link
Contributor

No description provided.

Copy link
Contributor

@cprice404 cprice404 left a comment

Choose a reason for hiding this comment

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

LGTM just one quick question

@@ -0,0 +1,3 @@
python3 -m venv client_sdk_python_env
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this file for devs, or for github workflows?

Copy link
Contributor Author

@gautamomento gautamomento Oct 26, 2021

Choose a reason for hiding this comment

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

This is for dev setup. I will rename this file and put local in the script name to make that clear. Thank you :)

Copy link
Contributor

Choose a reason for hiding this comment

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

cool - FWIW I think we should be steering devs towards using pyenv (or similar) for managing python installations. Using virtualenv here is a Very Good Thing, but I would still warn people against doing a system-install of python3 via brew, and prefer pyenv. I think this script will still work fine as-is but it might be worth adding a comment at the top of it to recommend pyenv :)

just a pet peeve of mine as I have spent so many hours helping people debug issues with system-wide installs of python (and ruby and gradle and node and...) over the years :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed prepare_local_env.sh -> prepare_local_dev_env.sh

@gautamomento gautamomento merged commit 36ee5bf into main Oct 26, 2021
@gautamomento gautamomento deleted the u/gautamomento/first-cut branch October 26, 2021 17:49
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.

3 participants