-
Notifications
You must be signed in to change notification settings - Fork 0
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
MNT #287 impose minimum Python version #306
Conversation
prjemian
commented
Jun 10, 2024
- close update minimum Python version for new_bluesky_instrument #287
Pull Request Test Coverage Report for Build 9456762787Details
💛 - Coveralls |
I will update the CHANGES.rst file for this PR, too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. Small comments. Also maybe a docstring will be useful for the file so that future contributors will know what this file is used for. As of right now this is unclear to me.
new_bluesky_instrument.py
Outdated
import requests | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason why this import is not global?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not global so the application does not crash under Python <3.9 with this exception. Instead, the more meaningful message is printed to use Python 3.9 or higher.
Could have put a try..except at the global level...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll rewrite it to the global part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New commit looks good. If you believe this to be a legitimate worry we will encounter during deployment we can implement this solution so that we have better control over the environment. Especially since the repository comes with an env file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure we need to put a maximum version. Minimum version for the bluesky environment is specified:
- python >=3.10 |
This is different than minimum version to run this installer. We'll want this soon since the current installer:
- fails for Python 3.6 and 3.7
- does not install the latest environment file
new_bluesky_instrument.py
Outdated
import requests | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
Pull Request Test Coverage Report for Build 9489045808Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9490715430Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9490764218Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!