Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Success Markers CLI #9824
Success Markers CLI #9824
Changes from 17 commits
778a69b
186a1e4
6a90fc2
a6e29e9
7ed690e
e4dd4fc
bf4030b
ab8d3f2
a70d06a
1d45b90
3aa624d
441ccc5
10d8360
65fad80
f630721
60cedb4
d6d1321
e686a3c
86ddf76
036a0a1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Does this need to be different from the "count" in
set_markers_first_n_arguments
?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.
Not strictly but it arises because of sub-parsers (already commented on them on another comment)
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.
Could this be done with argument i.e. with
add_mutually_exclusive_group
? Feels a bit weird doing it with subparsers...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.
I will check again, I looked into how
rasa test
does it and it looks like it uses sub-parsers (though I agree it's a bit odd). I think I could do it withadd_mutually_exclusive_group
and choices though.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.
So I had a look, and I'm not sure that's possible easily -
add_mutually_exclusive_group
lets us force one of the args in the group to be picked, but we have a situation where one option has 2 args that are allowed, one has one arg, and one has none. Those are all dependent on the value ofstrategy
(which we could force withchoices
), but I'm not sure we can do mutual exclusion another way without the use of sub-parsers here (which I agree looks quite nasty)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.
Should we add explicit unit tests for these new functions outside of the CLI tests?
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.
I get various type warnings in pycharm in this file. Should we fix these?
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.
👍 working on those as part of the config validation task -- we can ignore them for now