-
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
CLI: If gotocomputer
is not implemented, do not raise but show the remote_workdir
#6525
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6525 +/- ##
==========================================
+ Coverage 77.51% 77.82% +0.32%
==========================================
Files 560 562 +2
Lines 41444 41862 +418
==========================================
+ Hits 32120 32577 +457
+ Misses 9324 9285 -39 ☔ View full report in Codecov by Sentry. |
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.
Thanks @khsrali . All good, could you just add a quick test please.
echo.echo_report('going to the remote work directory...') | ||
os.system(command) | ||
except NotImplementedError: | ||
echo.echo_report(f'gotocomputer is not implemented for {transport.__str__()}') |
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.
When adding a variable in an f-string, the __str__
method is automatically called
echo.echo_report(f'gotocomputer is not implemented for {transport.__str__()}') | |
echo.echo_report(f'gotocomputer is not implemented for {transport}') |
def test_calcjob_gotocomputer(self, monkeypatch): | ||
"""Test verdi calcjob gotocomputer""" | ||
|
||
from unittest.mock import patch |
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 use unittest.mock
when there is monkeypatch
? Fine to keep the former if you prefer (even though we typically use the monkeypatch
fixture) but if you do, get rid of the monkeypatch
argument
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.
yes, you're right, I deleted monkeypatch
.
Here I preferred patch
because I could use it as context manager.
385d3df
to
45059a5
Compare
Thanks @khsrali |
…dateam#6525) Not all transport plugins implement the `gotocomputer` method. Instead of excepting, the remote working directory is now displayed.
gotocomputer
is not implemented inaiida-firecrest
meanwhile many users found this command useful to understand where is the remote directory of a particular process.As it is right now,
verdi calcjob gotocomputer
prints long scary errors.This PR suppresses a raise in such case and simply reports the working directory instead.
This was suggested by @mikibonacci who is actively using
aiida-firecrest
.