Fix interactive and conditional options in click #1666

giovannipizzi · 2018-06-19T14:17:44Z

to comply with the expected behaviour, now also tested.

Click uses the timestamp to see if the file has been modified in `click.edit()` but on some filesystems the timestamp has a precision of 1 second

to comply with the logic we expect. Tests have also been adapted as they were not always correct. Also, tests on empty_ok substituted with tests on empty string defaults as empty_ok has been removed from the code.

…k that all parameters passed to a builder are used.

E.g. when a previous option decides if the option should be printed or not Also fixed the tests. NOTE: if the wrong number of interactive options is passed in a test, this will enter an infinite loop and will hang the tests! To decide if we want to have a way to prevent this (e.g. a max_prompt_count variable?)

…ractive

giovannipizzi · 2018-06-19T21:48:05Z

@sphuber @ltalirz I think the hanging in my case was due to an infinite loop in the prompt for interactive inputs in verdi code tests. I fixed it here (even is probably we might want to set a max_prompt_loops variable to avoid this in the future, at least in the tests?). If tests pass, then the problem that @ltalirz noticed is independent of this

…ractive

because now the 'verdi node description' was getting by default -D "" and therefore was setting an empty string rather than getting the current description

codecov-io · 2018-06-19T22:36:02Z

Codecov Report

Merging #1666 into verdi will increase coverage by <.01%.
The diff coverage is 80.89%.

@@           Coverage Diff            @@
##           verdi   #1666      +/-   ##
========================================
+ Coverage   60.2%   60.2%   +<.01%     
========================================
  Files        294     297       +3     
  Lines      34065   34091      +26     
========================================
+ Hits       20509   20526      +17     
- Misses     13556   13565       +9

Impacted Files	Coverage Δ
aiida/cmdline/params/options/conditional.py	`100% <100%> (ø)`	⬆️
aiida/cmdline/commands/code.py	`34.77% <100%> (ø)`	⬆️
aiida/cmdline/commands/node.py	`50.88% <100%> (ø)`	⬆️
aiida/cmdline/params/options/__init__.py	`100% <100%> (ø)`	⬆️
aiida/cmdline/params/types/shebang.py	`41.66% <41.66%> (ø)`
aiida/cmdline/params/types/nonemptystring.py	`50% <50%> (ø)`
aiida/utils/error_accumulator.py	`83.33% <83.33%> (ø)`
aiida/cmdline/params/options/interactive.py	`75.25% <93.33%> (+2.53%)`	⬆️
aiida/control/code.py	`74.41% <94.44%> (-0.59%)`	⬇️
aiida/cmdline/params/types/identifier.py	`85.1% <0%> (-2.13%)`	⬇️
... and 5 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 f685844...cd12859. Read the comment docs.

ltalirz · 2018-06-20T05:50:47Z

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

-        os.environ['VISUAL'] = 'vim -cwq'
-        os.environ['EDITOR'] = 'vim -cwq'
+        os.environ['VISUAL'] = 'sleep 1; vim -cwq'
+        os.environ['EDITOR'] = 'sleep 1; vim -cwq'


since time is of the essence for unit tests, what about sleep 0.1?

Unfortunately the time precision on my Mac is 1 second, so a time >=1s is needed... Of course better approaches are accepted (even if probably they should become fixes to click.edit()

I made a PR pallets/click#1050 at click to fix this.
Once merged and released we can

remove the sleep 1

I would also substitute vim with sed (slimmer, no "window" open, ...) as I did in that PR

vim was chosen because it is a file editor and therefore closer to the intended usage than the stream editor sed. Since this test turned out to be fragile, I agree that maybe sed is a better compromise between test reliability and testing the right thing.

ltalirz · 2018-06-20T06:23:20Z

Are there any particular things you want us to look at?
I'm not sure I'll have the time to read and understand everything.

giovannipizzi · 2018-06-20T06:43:46Z

@ltalirz not really, probably only if you are happy with the new (slightly changed) behaviour of the InteractiveOption (should be more consistent with click default behaviour and require less custom code) as tested by the tests, the removal of the empty_ok custom parameter the added prompt_fn parameter to decide whether to prompt or not for a parameter (before this was merged together with required_fn but I think required_fn serves a different purpose, is specified in ConditionalOption while prompt_fn makes more sense for InteractiveOption).

ltalirz · 2018-06-20T07:00:18Z

Thanks for pointing this out.
It seems like in the PR (in code.py) there are no cases where prompt_fn and required_fn differ (correct me if I'm wrong). Could you please document somewhere (can be in the code, of course) how to use them and, in particular, give an idea of when they should differ?

giovannipizzi · 2018-06-20T07:43:54Z

In most cases I think they should also go together.
The point is

required_fn is about "is this parameter required" depending on the value of other params. This is implemented in ConditionalOption.
prompt_fn is about "should I prompt for this value if in interactive mode" and only makes sense in InteractiveOption (that inherits from ConditionalOption, but wouldn't make sense in that class, as it's specific to Interactive options).

I think the two should remain separate as concepts.
It is true that in most cases I have in mind, if I have a prompt_fn that I would like to have also a required_fn, but I preferred to avoid some automagic mangling that pass internally required_fn if prompt_fn is provided, to make things clear, and also to avoid to have to make complex decisions if the user passes both.

Some usecases where the two might differ:

the variable is anyway optional, so I just need to specify prompt_fn
even if I prompt for the variable, the conditions under which it is required are a subset of those under which I want to prompt

sphuber · 2018-06-20T07:45:03Z

aiida/cmdline/params/options/interactive.py

+            param_decls=None,
+            switch=None,  #
+            prompt_fn=None,
+            # empty_ok=False,


Probably best to remove the commented line and trailing comment character

ltalirz · 2018-06-20T07:46:10Z

@giovannipizzi Fine with the implementation, just put your explanations (and also the examples where they might differ) somewhere where one can find them

sphuber

Minor comments, but looks good otherwise

sphuber · 2018-06-20T07:46:03Z

aiida/cmdline/params/options/interactive.py


-        # super
+        # call super


If important to explain why we call super and why here than maybe we should write that as a comment. As it stands it is not very informative and I would remove it

As required in the comments of #1666, and also fixed a couple of other minor things required in the comments (mainly about comments)

giovannipizzi · 2018-06-20T08:10:35Z

@ltalirz @sphuber done

giovannipizzi · 2018-06-20T08:29:16Z

@ltalirz this lays the needed classes to implement verdi code setup that you were asking about.

Note to be careful if you change the command, as tests might hang if you change the order/validation/number of parameters (see my previous comment).

sphuber · 2018-06-20T08:35:44Z

This should have been squashed and merged, not normally merged. Now we have a bunch of non-sensical commits in the branch. Please let's use squash properly when we can use it

giovannipizzi · 2018-06-20T08:36:50Z

Sorry, you are right, I clicked on merge out of habit...

DropD · 2018-06-20T08:30:31Z

aiida/cmdline/params/options/interactive.py


-        # help option was passed on the cmdline
-        if ctx.params.get('help'):
-            return self.after_callback(ctx, param, value)


Is it tested somewhere that removing this never causes an InteractiveOption to enter the prompt loop before --help gets processed?

If it wasn't tested before, then no. It wasn't clear to me the purpose of that logic. Please feel free to add a test for what you have in mind.

DropD · 2018-06-20T08:41:25Z

aiida/control/code.py

-        code.set_prepend_text(self.prepend_text)
-        code.set_append_text(self.append_text)
+        # Complain if there are keys that are passed but not used
+        if passed_keys - used:


With the addition of this check it becomes necessary to be able to remove the additional keys.
Usecase:

ipython >> builder = CodeBuilder() ipython >> builder.label = ... # ... lines and lines of generating and adding the other keys ipython >> builder.extraneous_key = ... ipython >> builder.new() CodeValidationError ... ipython >> # Now the builder object that has been prepared in arduous work has become useless ipython >> # And we need to start from scratch!

I will turn this into an issue, seeing as the commit has already been merged.

thanks.
I think it's just a matter of implementing __del__ for attributes and forwarding the del call to the internal dictionary?

DropD and others added 3 commits June 19, 2018 13:07

add tests for missing behaviour

42281d0

Fixing a bug with click.edit

5b8f241

Click uses the timestamp to see if the file has been modified in `click.edit()` but on some filesystems the timestamp has a precision of 1 second

Changing the logic of Interactive options and Conditional options

44c5fe4

to comply with the logic we expect. Tests have also been adapted as they were not always correct. Also, tests on empty_ok substituted with tests on empty string defaults as empty_ok has been removed from the code.

giovannipizzi requested review from DropD and sphuber June 19, 2018 14:17

giovannipizzi added 4 commits June 19, 2018 16:36

Add some nitpick exceptions for click types and fix the logic to chec…

c0d7950

…k that all parameters passed to a builder are used.

The nitpick exception was incorrect

eeab898

Merge branch 'verdi' of github.com:aiidateam/aiida_core into fix-inte…

09b046a

…ractive

giovannipizzi added 2 commits June 19, 2018 23:49

Merge branch 'verdi' of github.com:aiidateam/aiida_core into fix-inte…

b6acc08

…ractive

The change done in the default for description was making a test fail

de6a473

because now the 'verdi node description' was getting by default -D "" and therefore was setting an empty string rather than getting the current description

ltalirz reviewed Jun 20, 2018

View reviewed changes

sphuber reviewed Jun 20, 2018

View reviewed changes

Added documentation for prompt_fn

cd12859

As required in the comments of #1666, and also fixed a couple of other minor things required in the comments (mainly about comments)

ltalirz self-requested a review June 20, 2018 08:18

ltalirz approved these changes Jun 20, 2018

View reviewed changes

giovannipizzi merged commit 7b0237d into verdi Jun 20, 2018

giovannipizzi deleted the fix-interactive branch June 20, 2018 08:28

DropD reviewed Jun 20, 2018

View reviewed changes

sphuber mentioned this pull request Dec 2, 2020

Config: make writing to disk as atomic as possible #4607

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix interactive and conditional options in click #1666

Fix interactive and conditional options in click #1666

giovannipizzi commented Jun 19, 2018

giovannipizzi commented Jun 19, 2018

codecov-io commented Jun 19, 2018 •

edited

Loading

ltalirz Jun 20, 2018

giovannipizzi Jun 20, 2018

giovannipizzi Jun 20, 2018

DropD Jun 20, 2018

ltalirz commented Jun 20, 2018

giovannipizzi commented Jun 20, 2018

ltalirz commented Jun 20, 2018 •

edited

Loading

giovannipizzi commented Jun 20, 2018

sphuber Jun 20, 2018

ltalirz commented Jun 20, 2018

sphuber left a comment

sphuber Jun 20, 2018

giovannipizzi commented Jun 20, 2018

giovannipizzi commented Jun 20, 2018

sphuber commented Jun 20, 2018

giovannipizzi commented Jun 20, 2018

DropD Jun 20, 2018

giovannipizzi Jun 20, 2018

DropD Jun 20, 2018

DropD Jun 20, 2018

giovannipizzi Jun 20, 2018

Fix interactive and conditional options in click #1666

Fix interactive and conditional options in click #1666

Conversation

giovannipizzi commented Jun 19, 2018

giovannipizzi commented Jun 19, 2018

codecov-io commented Jun 19, 2018 • edited Loading

Codecov Report

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 20, 2018

giovannipizzi commented Jun 20, 2018

ltalirz commented Jun 20, 2018 • edited Loading

giovannipizzi commented Jun 20, 2018

Choose a reason for hiding this comment

ltalirz commented Jun 20, 2018

sphuber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giovannipizzi commented Jun 20, 2018

giovannipizzi commented Jun 20, 2018

sphuber commented Jun 20, 2018

giovannipizzi commented Jun 20, 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

codecov-io commented Jun 19, 2018 •

edited

Loading

ltalirz commented Jun 20, 2018 •

edited

Loading