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

✨ NEW: Allow for CLI usage via python -m aiida #5356

Merged
merged 4 commits into from
Feb 17, 2022

Conversation

chrisjsewell
Copy link
Member

@chrisjsewell chrisjsewell commented Feb 16, 2022

This allows for advanced use-cases, where one may wish to directly specify the python interpreter to use.

$ python -m aiida
Usage: python -m aiida [OPTIONS] COMMAND [ARGS]...

  The command line interface of AiiDA.

Options:
  -p, --profile PROFILE           Execute the command for this profile instead
                                  of the default profile.
  -v, --verbosity [notset|debug|info|report|warning|error|critical]
                                  Set the verbosity of the output.
  --version                       Show the version and exit.
  --help                          Show this message and exit.

Commands:

@chrisjsewell
Copy link
Member Author

Not sure if/how we want to add a test 🤷

@sphuber
Copy link
Contributor

sphuber commented Feb 16, 2022

Not sure if/how we want to add a test shrug

You could add a simple test in the tests.sh bash script that calls python -m aiida and checks that the return exit code is 0 and the content printed to stdout contains Usage: python -m aiida [OPTIONS] COMMAND [ARGS]... for example. Should be enough no?

@chrisjsewell
Copy link
Member Author

Should be enough no?

yeh, might as well 👍

Copy link
Contributor

@csadorf csadorf left a 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.

@chrisjsewell
Copy link
Member Author

Added the test to verdi.sh

OUTPUT=$(python -m aiida 2>&1)
retVal=$?
if [ $retVal -ne 0 ]; then
echo "'python -m aiida' exitted with code $retVal"
Copy link
Contributor

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.

Copy link
Member Author

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

Copy link
Contributor

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

changed it

Copy link
Contributor

@sphuber sphuber left a 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

.github/workflows/verdi.sh Outdated Show resolved Hide resolved
.github/workflows/verdi.sh Outdated Show resolved Hide resolved
.github/workflows/verdi.sh Outdated Show resolved Hide resolved
.github/workflows/verdi.sh Show resolved Hide resolved
.github/workflows/verdi.sh Outdated Show resolved Hide resolved
Co-authored-by: Sebastiaan Huber <mail@sphuber.net>
@chrisjsewell chrisjsewell requested a review from sphuber February 17, 2022 12:57
@chrisjsewell chrisjsewell requested a review from csadorf February 17, 2022 13:05
@chrisjsewell chrisjsewell merged commit a760390 into aiidateam:develop Feb 17, 2022
@chrisjsewell chrisjsewell deleted the add_main branch February 17, 2022 13:23
OUTPUT=$(python -m aiida 2>&1)
RETVAL=$?
echo $OUTPUT
if [ $RETVAT -ne 0 ]; then
Copy link
Contributor

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 :)

Copy link
Member Author

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 😅

Copy link
Contributor

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

Copy link
Member Author

Choose a reason for hiding this comment

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

cheers!

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