Migrate verdi code #1655

ltalirz · 2018-06-18T17:15:25Z

Migrating verdi code commands to click

verdi code delete
verdi code hide
add verdi code reveal
add verdi code relabel
verdi code list
deprecate verdi code update (suggests to use verdi code duplicate)
deprecate verdi code rename (still works for the moment, forwards to verdi code relabel)
Added the corresponding source file to pre-commit tests
Add unit tests for all commands
Migrate old tests for verdi code list
fix Code._hide and Code._reveal should not be protected #1660
fix "verdi code show" no-argument error message ambiguous #698
verdi code setup
- move over some help strings from old implementation
- change order of prompts: ask for input plugin before asking computer-related questions

Still to do:

make use of the new deprecated decorator for code update and code rename
figure out whether code coverage in computer is really going down - I believe this should be fixed by the proper testing done in the migrated verdi computer commands

fix verdi code setup
- bring back info text? NO
```
At any prompt, type ? to get some help.
---------------------------------------
```
- Should the => prefix be brought back? NO
- When hitting enter on empty/invalid input, should suggest to use ? for help (will be fixed in make prompts yellow (and add help for invalid args to prompts) #1684)
- Check on absolute path now happens only at the very end (should happen when you enter it)

For verdi code duplicate I'll add a separate pull request
* add option to hide previous code
* potentially make use of callback to fill values of one option by value of other options (ask martin). It might be better though to use context forwarding to verdi code setup

fix aiidateam#1614 Code.get_code_label is the same as Code.label, but you can get the full code label (including computer name) by specifying 'full=True'

* migrate verdi code rename to click * add verdi code relabel (verdi code rename deprecated) * add Code.relabel function * deprecate verdi code update

+ fix 'code show' and add test

codecov-io · 2018-06-19T09:41:06Z

Codecov Report

Merging #1655 into verdi will increase coverage by 0.02%.
The diff coverage is 90.38%.

@@            Coverage Diff            @@
##           verdi    #1655      +/-   ##
=========================================
+ Coverage   60.9%   60.92%   +0.02%     
=========================================
  Files        298      299       +1     
  Lines      34054    33721     -333     
=========================================
- Hits       20741    20546     -195     
+ Misses     13313    13175     -138

Impacted Files	Coverage Δ
aiida/cmdline/params/arguments/__init__.py	`100% <100%> (ø)`	⬆️
aiida/cmdline/params/options/__init__.py	`100% <100%> (ø)`	⬆️
aiida/cmdline/params/types/path.py	`83.33% <83.33%> (ø)`
aiida/orm/implementation/general/code.py	`76.79% <85%> (+10.57%)`	⬆️
aiida/cmdline/commands/code.py	`90.64% <91.8%> (+55.86%)`	⬆️
aiida/orm/implementation/sqlalchemy/computer.py	`60.32% <0%> (-21.2%)`	⬇️
aiida/orm/implementation/general/computer.py	`50.58% <0%> (-20.59%)`	⬇️
aiida/orm/implementation/django/computer.py	`65.95% <0%> (-14.9%)`	⬇️
aiida/cmdline/commands/computer.py	`32.66% <0%> (-11.28%)`	⬇️
aiida/backends/sqlalchemy/models/computer.py	`84.28% <0%> (-5.72%)`	⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15145bd...5e0066b. Read the comment docs.

+ fix bug introduced in merge

Check for absolute path was only done at the very end.

ltalirz · 2018-06-25T17:39:04Z

@sphuber should be ready for you to have a look now

sphuber · 2018-06-25T17:45:29Z

.pre-commit-config.yaml

@@ -19,12 +19,14 @@
        aiida/cmdline/tests/.*.py|  # the .*.py (.* matches everything)
        aiida/cmdline/commands/calculation.py|
        aiida/cmdline/commands/daemon.py|
+        aiida/cmdline/commands/code.py|


Please also add aiida.backends.tests.cmdline.commands.test_code.py

sphuber · 2018-06-25T17:47:25Z

aiida/backends/tests/cmdline/commands/test_code.py

+        result = self.runner.invoke(hide, [str(self.code.pk)])
+        self.assertIsNone(result.exception)
+
+        self.assertTrue(self.code.get_extra(self.code.HIDDEN_KEY))


Probably better to use the method _is_hidden. The linter will then complain that a protected method is called, but I think that method should be public anyway. So it would be great if you could make _is_hidden public. While we are at it, I think we should also make _reveal and _hide public

yes, I had opened an issue for that #1660 but nobody replied.
If you agree, I'll proceed with it.

sphuber · 2018-06-25T17:51:18Z

aiida/orm/implementation/general/code.py

-                                                             self.pk, self.uuid)
+        return computer_str
+
+    def get_label(self, full=True):


Is there a specific point to have this construction for get_label. Given that default is True, wouldn't it make more sense to just rename this function to get_full_label without the args, since the normal label should just be gotten with Codel.label.

Good catch, the intention was full=False as default. Would you agree with this change?

Why not have a property like:

@property def full_label(self): return '{}@{}'.format(self.label, self.computer.name)

That seems a lot cleaner, because I don't see the point of having the method with the argument, if you already have a perfectly good property for label

sphuber · 2018-06-25T17:53:38Z

aiida/cmdline/commands/code.py

+#                            continue
+#                        else:
+#                            newlines.append(input_txt)
+#                except EOFError:


Why is this commented and not just removed?

Before deleting old code I wanted to make sure all useful bits have been carried over.
I've now removed a bunch more comments, what is still there I'd like to leave until verdi code duplicate is implemented

leaving in a few comments for verdi code duplicate

fix aiidateam#1660 * Code._hide => Code.hide * Code._reveal => Code.reveal * Code._is_hidden => Code.is_hidden

replacing function with parameter full by property.

ltalirz · 2018-06-25T20:42:58Z

I'm now looking into one last thing which is the naming issue of local codes.
I'll let you know once this is done, please do not merge before.

fix aiidateam#127 Adjust help strings similar to suggestion in aiidateam#127 (comment)

ask for input plugin before asking for anything related to the computer where the code is stored.

ltalirz · 2018-06-25T21:44:55Z

@sphuber Done.

ltalirz · 2018-06-26T17:05:02Z

@sphuber Should be fine from my side - let me know if there's anything else you'd like to change

sphuber · 2018-06-26T17:06:15Z

Did this rely on Rico's PR of the prompts? Or it does not matter in which order they are merged?

ltalirz · 2018-06-26T17:08:11Z

No, they should be independent.
It's possible in principle that some new tests will fail (because of the addition of the help string) but maybe not.
We'll see that after merging.

ltalirz added 11 commits June 18, 2018 10:21

migrate verdi code delete

69208b6

add hide, reveal + unit tests

08b5f07

add get_code_label convenience function

3eb365c

fix aiidateam#1614 Code.get_code_label is the same as Code.label, but you can get the full code label (including computer name) by specifying 'full=True'

Merge remote-tracking branch 'origin/verdi' into migrate_verdi_code

13499bb

migrate code rename, add code relabel

0da9664

* migrate verdi code rename to click * add verdi code relabel (verdi code rename deprecated) * add Code.relabel function * deprecate verdi code update

harden tests

a60b9ca

migrate verdi code list to click

a87dbc6

Merge branch 'verdi' into migrate_verdi_code

8588d8f

Merge branch 'verdi' into migrate_verdi_code

b6d6c14

add code.py to pre-commit

d67294d

+ fix 'code show' and add test

migrate old tests for code list

fb4a4ca

Merge branch 'verdi' into migrate_verdi_code

e0486c7

DropD added topic/verdi type/accepted feature approved feature request labels Jun 20, 2018

ltalirz added 4 commits June 25, 2018 09:31

Merge branch 'verdi' into migrate_verdi_code

f22ad75

use deprecated_command decorator

daaefdc

+ fix bug introduced in merge

add immediate check for absolute path

bb56eef

Check for absolute path was only done at the very end.

fix doc warning

5e71ce0

ltalirz changed the title ~~[WIP] Migrate verdi code~~ Migrate verdi code Jun 25, 2018

ltalirz requested a review from sphuber June 25, 2018 14:24

sphuber requested changes Jun 25, 2018

View reviewed changes

ltalirz added 5 commits June 25, 2018 22:02

remove some commented lines

29e32cc

leaving in a few comments for verdi code duplicate

check Code._is_hidden() instead of HIDDEN_KEY

f96dc6f

remove protection from Code.hide and Code.reveal

7af31a4

fix aiidateam#1660 * Code._hide => Code.hide * Code._reveal => Code.reveal * Code._is_hidden => Code.is_hidden

fix leftover _hide

516beec

replace Code.get_label() by Code.full_label

deff862

replacing function with parameter full by property.

adjust help strings for local/remote codes

5f2cf72

fix aiidateam#127 Adjust help strings similar to suggestion in aiidateam#127 (comment)

change order of prompts

5e0066b

ask for input plugin before asking for anything related to the computer where the code is stored.

sphuber approved these changes Jun 26, 2018

View reviewed changes

sphuber merged commit b115e30 into aiidateam:verdi Jun 26, 2018

sphuber mentioned this pull request Jun 26, 2018

verdi click: migrate verdi code #1580

Closed

This was referenced Jun 26, 2018

Code._hide and Code._reveal should not be protected #1660

Closed

"verdi code show" no-argument error message ambiguous #698

Closed

sphuber mentioned this pull request Jun 27, 2018

Convenience function to get full Code label #1614

Closed

ltalirz deleted the migrate_verdi_code branch August 28, 2018 08:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate verdi code #1655

Migrate verdi code #1655

ltalirz commented Jun 18, 2018 •

edited

Loading

codecov-io commented Jun 19, 2018 •

edited

Loading

ltalirz commented Jun 25, 2018

sphuber Jun 25, 2018

ltalirz Jun 25, 2018

sphuber Jun 25, 2018

ltalirz Jun 25, 2018

sphuber Jun 25, 2018

ltalirz Jun 25, 2018

sphuber Jun 25, 2018

ltalirz Jun 25, 2018

sphuber Jun 25, 2018

ltalirz Jun 25, 2018

ltalirz commented Jun 25, 2018

ltalirz commented Jun 25, 2018

ltalirz commented Jun 26, 2018

sphuber commented Jun 26, 2018

ltalirz commented Jun 26, 2018

Migrate verdi code #1655

Migrate verdi code #1655

Conversation

ltalirz commented Jun 18, 2018 • edited Loading

codecov-io commented Jun 19, 2018 • edited Loading

Codecov Report

ltalirz commented Jun 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltalirz commented Jun 25, 2018

ltalirz commented Jun 25, 2018

ltalirz commented Jun 26, 2018

sphuber commented Jun 26, 2018

ltalirz commented Jun 26, 2018

ltalirz commented Jun 18, 2018 •

edited

Loading

codecov-io commented Jun 19, 2018 •

edited

Loading