Fixed the yes/no prompt in verdi computer delete #6624
Conversation
|
I am sorry for the back and forth but reading now the conversation in issue #6422 more carefully and looking at the deletion in other parts of the code. We can change it to |
agoscinski
force-pushed
the
fix/yes/no-prompt-verdi-computer-delete
branch
from
ce1ff59 to
ccc2832
Compare
agoscinski
force-pushed
the
fix/yes/no-prompt-verdi-computer-delete
branch
from
1225c23 to
1f5aea4
Compare
Ohk @agoscinski I will do '[y/N]' |
|
@agoscinski |
|
No worries I take over. It should now work. |
|
I want to contribute more to organisation so I want to install the codebase locally |
agoscinski
force-pushed
the
fix/yes/no-prompt-verdi-computer-delete
branch
from
7a9e03d to
e3f8bef
Compare
agoscinski
force-pushed
the
fix/yes/no-prompt-verdi-computer-delete
branch
from
e3f8bef to
a33cfb2
Compare
|
Sure can you contact me on https://aiida.discourse.group ? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6624 +/- ##
==========================================
+ Coverage 77.51% 77.90% +0.40%
==========================================
Files 560 567 +7
Lines 41444 42178 +734
==========================================
+ Hits 32120 32856 +736
+ Misses 9324 9322 -2 ☔ View full report in Codecov by Sentry. |
To simplify the testing of 'y' and 'yes' case without rerunning unecessary database operations the tests for `verdi computer delete` have been refactored.
|
Thank you sir |
There was a problem hiding this comment.
Thanks a lot @Krishnabhujade and @agoscinski ,
Some minor comments
| def test_computer_delete_nonexisting_computer(self): | ||
| """Test if `verdi computer delete` command works correctly for a nonexisting computer""" | ||
| # See if the command complains about not getting an nonexisting computer | ||
| self.cli_runner(computer_delete, ['computer_that_does_not_exist'], raises=True) |
There was a problem hiding this comment.
Please check explicitly if the raise is due to NotExistent.
Also please pass user_input=y I know here it might not matter because the query comes before click.confirm,
however, later if someone changes the order of the logic in the code then test might silently pass..
Better to be precise.
Thanks
| @@ -634,7 +634,7 @@ def _dry_run_callback(pks): | |||
| if pks: | |||
| echo.echo_report('The nodes with the following pks would be deleted: ' + ' '.join(map(str, pks))) | |||
| echo.echo_warning(f'YOU ARE ABOUT TO DELETE {len(pks)} NODES! THIS CANNOT BE UNDONE!') | |||
| confirm = click.prompt('Shall I continue? [yes/N]', type=str) == 'yes' | |||
| confirm = click.confirm('Shall I continue?', default=False) | |||
| if not confirm: | |||
| raise click.Abort | |||
| return not confirm | |||
There was a problem hiding this comment.
May I take the chance to improve the message at line 642:
(if there are no nodes, this is a bit annoying saying "0 nodes marked for deletion")
if node_pks: delete_nodes(node_pks, dry_run=dry_run or _dry_run_callback)| orm.Computer.collection.get(label=label) | ||
| # Safety check in case of --dry-run | ||
| options = [label, '--dry-run'] | ||
| self.cli_runner(computer_delete, options) |
There was a problem hiding this comment.
| self.cli_runner(computer_delete, options) | |
| self.cli_runner(computer_delete, options, user_input="y") |
| # Setup a computer to delete during the test | ||
| label = 'computer_for_test_label' | ||
| orm.Computer( | ||
| def test_computer_delete_existing_computer(self): |
There was a problem hiding this comment.
I found this test name kinda misleading for it's purpose
| def test_computer_delete_existing_computer(self): | |
| def test_computer_do_not_delete(self): |
This PR fixes the "yes/no" prompt in verdi computer delete
Fixes #6422