FIX: InteractiveOption Remove double call of normal callback
#5064
Conversation
In the ``InteractiveOption`` providing a normal callback is wrapped
in the method ``after_callback`` while the actual callback is replaced
by ``prompt_callback``. Now the following situation occurs when the
parameter should be prompted for:
1. ``prompt_loop`` is entered via ``prompt_callback`` and the value
is converted in ``safely_convert``
2. ``safely_convert`` also includes a call to
``prompt_callback``, where a call to ``after_callback`` is
performed
3. After a value succeeds the code goes back into the place, where
the ``prompt_loop`` was entered in ``prompt_callback``
and continues with an additional call to ``after_callback``
which already executed before
The fix here is to directly return after the prompt_loop finishes and
handle the case, where the value should not be prompted separately
Codecov Report
@@ Coverage Diff @@
## develop #5064 +/- ##
===========================================
+ Coverage 80.39% 80.39% +0.01%
===========================================
Files 529 529
Lines 36881 36881
===========================================
+ Hits 29645 29647 +2
+ Misses 7236 7234 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks for your contribution @janssenhenning . The solution looks fine at first look. Just have a small suggestion for the test that I think makes the chances for false positives smaller and is slightly easier to follow as to what is happening. Let me know what you think.
| :param value: the value passed for the parameter | ||
| :raises `click.BadParameter`: if the value is not a positive number | ||
| """ | ||
| click.echo('Validating Number') |
There was a problem hiding this comment.
I am wondering if we can make the testing of the output more robust, by including the value in the echo
| click.echo('Validating Number') | |
| click.echo(f'Validating {value}') |
| lines = result.output.split('\n') | ||
| #The callback should be called once per prompt | ||
| #where type conversion was successful | ||
| self.assertEqual(lines.count('Validating Number'), 3) |
There was a problem hiding this comment.
With the change of including the value being passed in the echo string, I think we can make the test more robust and make the changes on false positives smaller. Since the echo should be the only output (right?) we should also be able to check the entire output verbatim. So I would suggest something like the following:
| lines = result.output.split('\n') | |
| #The callback should be called once per prompt | |
| #where type conversion was successful | |
| self.assertEqual(lines.count('Validating Number'), 3) | |
| self.assertEqual(result.output, 'Validating string\nValidating -1\nValidating 1\n') |
There was a problem hiding this comment.
I agree that the test can be made more robust by including the value in the echo, but the change will not work as is, since for example 'Validating string' will not appear in the output. This value will fail when it is converted to a float so the callback will never be called.
There was a problem hiding this comment.
Ah I see, because it would already fail the validation at the type which is expecting a float. Well, by accident, I think that is actually good to keep the string value in, as it tests that that doesn't hit the callback as it shouldn't. So maybe all you have to change in my suggestion is just to remove Validate string\n from the expected output
There was a problem hiding this comment.
I will probably need to check specific lines of the output, since there is more stuff in result.output besides the 'Validating {value}' lines like the lines for entering the values for example but in principle yes
| """ | ||
| cmd = self.simple_command(type=float, callback=validate_positive_number_with_echo) | ||
| runner = CliRunner() | ||
| result = runner.invoke(cmd, [], input='string\n-1\n-1\n1\n') |
There was a problem hiding this comment.
If you accept my other suggestions, the input should also be updated.
| result = runner.invoke(cmd, [], input='string\n-1\n-1\n1\n') | |
| result = runner.invoke(cmd, [], input='string\n-1\n1\n') |
janssenhenning
deleted the
issue_5023_callback_called_twice_interactive_option
branch
…5064) In the `InteractiveOption` providing a normal callback is wrapped in the method `after_callback` while the actual callback is replaced by `prompt_callback`. Now the following situation occurs when the parameter should be prompted for: 1. `prompt_loop` is entered via `prompt_callback` and the value is converted in `safely_convert` 2. `safely_convert` also includes a call to `prompt_callback`, where a call to `after_callback` is performed 3. After a value succeeds the code goes back into the place, where the `prompt_loop` was entered in `prompt_callback` and continues with an additional call to `after_callback` which already executed before The fix here is to directly return after the `prompt_loop` finishes and handle the case, where the value should not be prompted separately Cherry-pick: 6f93253
Fixes #5023.
In the
InteractiveOptionproviding a normal callback is wrappedin the method
after_callbackwhile the actual callback is replacedby
prompt_callback. Now the following situation occurs when theparameter should be prompted for:
prompt_loopis entered viaprompt_callbackand the valueis converted in
safely_convertsafely_convertalso includes a call toprompt_callback, where a call toafter_callbackisperformed
the
prompt_loopwas entered inprompt_callbackand continues with an additional call to
after_callbackwhich already executed before
The fix here is to directly return after the prompt_loop finishes and
handle the case, where the value should not be prompted separately