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

check / install python version using pyenv #1

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

flatnine
Copy link

Checks the correct version of Python is installed and managed by Pyenv
If version is not installed attempts to install using pyenv
If version is not available prompts user to update pyenv

@flatnine flatnine requested a review from kthhrv March 31, 2023 13:16
@flatnine
Copy link
Author

flatnine commented Mar 31, 2023

Sorry asking you to check code style. I can add some tests.

@kthhrv
Copy link
Contributor

kthhrv commented Mar 31, 2023

a lot more code than I was expecting ;-)

if status:
raise BaseException(f'Failed to install python version {python_version}')
else:
print(f'- Python `{python_version}` installation successful ✅')
Copy link
Contributor

Choose a reason for hiding this comment

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

the first part of the message should print before the installs start and the tick be added when in succeeds

@@ -158,9 +162,13 @@ def python_setup_func(args):


def python_setup(app, branch, python_version):
try:
check_python_version_installed(python_version)
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel the logic shouldn't be inside the check function, ie if the check returns false here then call the install here

Copy link
Author

Choose a reason for hiding this comment

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

certainly makes the tests easier

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