-
Notifications
You must be signed in to change notification settings - Fork 192
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
✨ NEW: Allow for CLI usage via python -m aiida
#5356
Conversation
Not sure if/how we want to add a test 🤷 |
You could add a simple test in the |
yeh, might as well 👍 |
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.
Great! I'm also in favor of adding a basic smoke test for this.
e31474a
to
accff4b
Compare
Added the test to |
.github/workflows/verdi.sh
Outdated
OUTPUT=$(python -m aiida 2>&1) | ||
retVal=$? | ||
if [ $retVal -ne 0 ]; then | ||
echo "'python -m aiida' exitted with code $retVal" |
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.
Why not just exit 2
here as well? Is there a reason for forwarding the exit code from the test script? Seems slightly unexpected to me.
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.
Well my thinking was that the 2nd check, of the stdout, should also run, even if the exit code is not 0
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.
Why? But even so, I would still exit 2
on any failure. It just seems unintuitive to me that the test script would forward the exit code unless explicitly requested.
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.
changed it
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.
All good, just small change on variable style consistency and I would suggest to actually print some output even if all goes well. This will allow to check the logs and verify that something was actually run. Now we don't see any evidence of this check having been run
Co-authored-by: Sebastiaan Huber <mail@sphuber.net>
OUTPUT=$(python -m aiida 2>&1) | ||
RETVAL=$? | ||
echo $OUTPUT | ||
if [ $RETVAT -ne 0 ]; then |
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.
Uhm @chrisjsewell this may have been my mistake in the review suggestion, but there is a type here. If I promise to not merge your PRs anymore, will you promise to let me sign off on them first :)
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.
Doh, thats all your fault 😅
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.
Don't worry, I have a small PR coming up with some minor corrections that I can put this in as well
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.
cheers!
This allows for advanced use-cases, where one may wish to directly specify the python interpreter to use.