diff --git a/CHANGELOG.md b/CHANGELOG.md index 839db0b7..31618525 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Python version `3.10` tested during CI - Python version `3.10` added to package classifiers +- `should_ask` keyword argument for `Echo` and `Acknowledge` interactions ([#356](https://github.com/wayfair-incubator/columbo/issues/356)) ### Removed diff --git a/columbo/__init__.py b/columbo/__init__.py index e587e87c..32e1387c 100644 --- a/columbo/__init__.py +++ b/columbo/__init__.py @@ -10,6 +10,7 @@ BasicQuestion, Choice, Confirm, + Displayable, Echo, Interaction, Question, diff --git a/columbo/_interaction.py b/columbo/_interaction.py index 91797be6..2df2f607 100644 --- a/columbo/_interaction.py +++ b/columbo/_interaction.py @@ -19,7 +19,7 @@ Validator, ) -Interaction = Union["Echo", "Acknowledge", "Question"] +Interaction = Union["Displayable", "Question"] # Used by copy() implementations. Since some arguments can be None, None can't be used as the value to indicate that the @@ -39,60 +39,125 @@ def _or_default(value, default: T) -> T: return default if isinstance(value, _Sentinel) else value -class Echo: - """Display a message to the user.""" +def _should_ask_or_display(should_ask: Optional[ShouldAsk], answers: Answers) -> bool: + if should_ask is None: + return True + if callable(should_ask): + return should_ask(answers) + raise ValueError(f"Invalid value for should_ask: {should_ask}") + + +class Displayable(ABC): + """ + Base class for a message to the user that is displayed. + """ - def __init__(self, message: StaticOrDynamicValue[str]) -> None: + def __init__( + self, message: StaticOrDynamicValue[str], should_ask: Optional[ShouldAsk] = None + ) -> None: """ Initialize an instance. :param message: The message to be displayed to the user. If the value is callable, the argument passed in will be the answers that have been provided this far. + :param should_ask: If `None`, the message is displayed to the user. Otherwise, the callable will be passed the + answers that have been provided this far and should return `True` if the message should be displayed. """ self._message = message + self._should_ask = should_ask + + @abstractmethod + def display(self, answers: Answers) -> None: # pragma: no cover + pass + + def should_ask(self, answers: Answers) -> bool: + """ + Should the user be displayed this message. + + :param answers: The answers that have been provided this far. + :return: `True` if this message should be displayed + """ + return _should_ask_or_display(self._should_ask, answers) + + +class Echo(Displayable): + """Display a message to the user.""" + + def __init__( + self, message: StaticOrDynamicValue[str], should_ask: Optional[ShouldAsk] = None + ) -> None: + """ + Initialize an instance. + + :param message: The message to be displayed to the user. If the value is callable, the argument passed in will + be the answers that have been provided this far. + :param should_ask: If `None`, the message is displayed to the user. Otherwise, the callable will be passed the + answers that have been provided this far and should return `True` if the message should be displayed. + """ + super().__init__(message, should_ask) def display(self, answers: Answers) -> None: user_io.echo(to_value(self._message, answers, str)) def copy( - self, *, message: Possible[StaticOrDynamicValue[str]] = _NOT_GIVEN + self, + *, + message: Possible[StaticOrDynamicValue[str]] = _NOT_GIVEN, + should_ask: Possible[Optional[ShouldAsk]] = _NOT_GIVEN, ) -> "Echo": """ Create a new instance like this one, potentially with different values. :param message: The message to be displayed to the user. If the value is callable, the argument passed in will be the answers that have been provided this far. + :param should_ask: If `None`, the message is displayed. Otherwise, the callable will be passed the answers that + have been provided this far and should return `True` if the message should be displayed. :return: A newly constructed instance with the given values in place of the values of this instance. """ - return Echo(_or_default(message, self._message)) + return Echo( + _or_default(message, self._message), + should_ask=_or_default(should_ask, self._should_ask), + ) -class Acknowledge: +class Acknowledge(Displayable): """Display a message to the user and require the user to press ENTER to continue.""" - def __init__(self, message: StaticOrDynamicValue[str]) -> None: + def __init__( + self, message: StaticOrDynamicValue[str], should_ask: Optional[ShouldAsk] = None + ) -> None: """ Initialize an instance. :param message: The message to be displayed to the user. If the value is callable, the argument passed in will be the answers that have been provided this far. + :param should_ask: If `None`, the message is displayed to the user. Otherwise, the callable will be passed the + answers that have been provided this far and should return `True` if the message should be displayed. """ - self._message = message + super().__init__(message, should_ask) def display(self, answers: Answers) -> None: user_io.acknowledge(to_value(self._message, answers, str)) def copy( - self, *, message: Possible[StaticOrDynamicValue[str]] = _NOT_GIVEN + self, + *, + message: Possible[StaticOrDynamicValue[str]] = _NOT_GIVEN, + should_ask: Possible[Optional[ShouldAsk]] = _NOT_GIVEN, ) -> "Acknowledge": """ Create a new instance like this one, potentially with different values. :param message: The message to be displayed to the user. If the value is callable, the argument passed in will be the answers that have been provided this far. + :param should_ask: If `None`, the message is displayed. Otherwise, the callable will be passed the answers that + have been provided this far and should return `True` if the message should be displayed. :return: A newly constructed instance with the given values in place of the values of this instance. """ - return Acknowledge(_or_default(message, self._message)) + return Acknowledge( + _or_default(message, self._message), + should_ask=_or_default(should_ask, self._should_ask), + ) class Question(ABC): @@ -140,7 +205,9 @@ def cli_help(self) -> Optional[str]: return self._cli_help @abstractmethod - def ask(self, answers: Answers, no_user_input: bool = False) -> Answer: + def ask( + self, answers: Answers, no_user_input: bool = False + ) -> Answer: # pragma: no cover """ Prompt the user with this question. @@ -158,11 +225,7 @@ def should_ask(self, answers: Answers) -> bool: :param answers: The answers that have been provided this far. :return: `True` if this questions should be asked """ - if self._should_ask is None: - return True - if callable(self._should_ask): - return self._should_ask(answers) - raise ValueError(f"Invalid value for should_ask: {self._should_ask}") + return _should_ask_or_display(self._should_ask, answers) class Confirm(Question): @@ -556,7 +619,8 @@ def get_answers( for interaction in interactions: if isinstance(interaction, (Echo, Acknowledge)): - interaction.display(result) + if interaction.should_ask(result): + interaction.display(result) elif isinstance(interaction, (Confirm, Choice, BasicQuestion)): if interaction.should_ask(result): result[interaction.name] = interaction.ask(result, no_user_input) diff --git a/docs/examples/branching_story.py b/docs/examples/branching_story.py index 0c351240..c7e550e4 100644 --- a/docs/examples/branching_story.py +++ b/docs/examples/branching_story.py @@ -33,18 +33,24 @@ def outcome(answers: columbo.Answers) -> str: options=["left", "right"], default="left", ), + columbo.Echo( + "You step into a short hallway and the door closes behind you, refusing to open again. " + "As you walk down the hallway, there is a small side table with a key on it.", + should_ask=went_left, + ), columbo.Confirm( "has_key", - "You step into a short hallway and the door closes behind you, refusing to open again. " - "As you walk down the hallway, there is a small side table with a key on it.\n" "Do you pick up the key before going through the door at the other end?", should_ask=went_left, default=True, ), + columbo.Echo( + "You step into smaller room and the door closes behind, refusing to open again. " + "The room has a single door on the opposite side of the room and a work bench with a hammer on it.", + should_ask=went_right, + ), columbo.Confirm( "has_hammer", - "You step into smaller room and the door closes behind, refusing to open again. " - "The room has a single door on the opposite side of the room and a work bench with a hammer on it.\n" "Do you pick up the hammer before going through the door at the other side?", should_ask=went_right, default=True, diff --git a/docs/examples/optional_questions.py b/docs/examples/optional_questions.py index 903dc8b4..9455aeb3 100644 --- a/docs/examples/optional_questions.py +++ b/docs/examples/optional_questions.py @@ -7,6 +7,10 @@ def user_has_dog(answers: columbo.Answers) -> bool: interactions = [ columbo.Confirm("has_dog", "Do you have a dog?", default=True), + columbo.Echo( + "Because you have have a dog, we want to ask you some more questions.", + should_ask=user_has_dog, + ), columbo.BasicQuestion( "dog_name", "What is the name of the dog?", diff --git a/docs/examples/optional_questions_with_value_if_not_asked.py b/docs/examples/optional_questions_with_value_if_not_asked.py index aa89b40f..d65d206f 100644 --- a/docs/examples/optional_questions_with_value_if_not_asked.py +++ b/docs/examples/optional_questions_with_value_if_not_asked.py @@ -7,6 +7,10 @@ def user_has_dog(answers: columbo.Answers) -> bool: interactions = [ columbo.Confirm("has_dog", "Do you have a dog?", default=True), + columbo.Echo( + "Because you have have a dog, we want to ask you some more questions.", + should_ask=user_has_dog, + ), columbo.BasicQuestion( "dog_name", "What is the name of the dog?", diff --git a/docs/index.md b/docs/index.md index 5ecfc136..4d1a08fd 100644 --- a/docs/index.md +++ b/docs/index.md @@ -13,12 +13,12 @@ * Yes or No * Multiple choice * Open-ended -* Validate the response provided by the user. +* Validate the response provided by the user * Use answers from earlier questions: * As part of the text of a question * As part of the text of a default value - * To decide if a question should be skipped -* Accept answers from the command line in addition to prompting the user. + * To decide if a question should be skipped or a message should be displayed +* Accept answers from the command line in addition to prompting the user ## Example diff --git a/docs/usage-guide/interactions.md b/docs/usage-guide/interactions.md index 291c43b0..0bccf731 100644 --- a/docs/usage-guide/interactions.md +++ b/docs/usage-guide/interactions.md @@ -15,7 +15,13 @@ ### Echo & Acknowledge -`Echo` and `Acknowledge` both accept `message` as their only argument. This is the message to be displayed to the user. +`Echo` and `Acknowledge` both accept the following arguments. + +* `message`: The message to be displayed to the user. +* `should_ask`: Optional. When given, the argument should be a function that accepts an `Answers` dictionary and returns + `True` or `False`. Returning `True` indicates that the message should be displayed. Returning `False` will skip the + message and not present it to the user. See [Optional Questions & Branching][optional-questions] for more details. + ### All Questions diff --git a/docs/usage-guide/optional-questions-and-branching.md b/docs/usage-guide/optional-questions-and-branching.md index da641f1f..2689ca47 100644 --- a/docs/usage-guide/optional-questions-and-branching.md +++ b/docs/usage-guide/optional-questions-and-branching.md @@ -1,8 +1,11 @@ -## Only Asking Some Questions +## Only Prompting Some Interactions -There are situations where a question should be asked some times, but not all the time. For example, a program that collects -information about a user's pets should not ask the user for the dog's name and breed if the user said they do not have a -dog. The `should_ask` argument that is present on each question provides a way to achieve this functionality. +There are situations where a question should be asked some times, but not all the time. Or, a message should be displayed some times. +For example, a program that collects information about a user's +pets should not ask the user for the dog's name and breed if the +user said they do not have a dog. If the user has a dog, the program +may want to display a special message. The `should_ask` argument that +is present on each interaction provides a way to achieve this functionality. Similarly, `should_ask` can be used to provide branching paths to the user. An example of these branching paths is a [Choose Your Own Adventure][choose-your-own-adventure] story. The story provides the reader with choices during the @@ -16,7 +19,7 @@ adventure. These choices introduce diverging paths of interactions that may or m ### Optional Questions The following is a basic example that has two optional questions that are not asked based on the answer to the first -question. +question. It also has an optional message that is only displayed based on the answer to the first question. ```python {!examples/optional_questions.py!} diff --git a/tests/interaction_test.py b/tests/interaction_test.py index 3c2ca214..d885dd4b 100644 --- a/tests/interaction_test.py +++ b/tests/interaction_test.py @@ -24,6 +24,7 @@ SOME_OPTIONS, SOME_OTHER_BOOL, SOME_STRING, + SampleDisplayable, SampleQuestion, some_dynamic_bool, some_dynamic_default, @@ -450,6 +451,25 @@ def test_should_ask__invalid_type__exception(): ) +@pytest.mark.parametrize( + "description,should_ask,expected_result", + [("not set", None, True), ("dynamic", some_dynamic_bool, SOME_OTHER_BOOL)], +) +def test_displayable_should_ask__expected_result( + should_ask, expected_result, description +): + result = SampleDisplayable(SOME_STRING, should_ask=should_ask).should_ask( + SOME_ANSWERS + ) + + assert result == expected_result, description + + +def test_displayable_should_ask__invalid_type__exception(): + with pytest.raises(ValueError): + SampleDisplayable(SOME_STRING, should_ask=object()).should_ask(SOME_ANSWERS) + + def test_get_answers__unknown_interaction_provided(): """ An unsupported interaction type should raise a ValueError @@ -461,18 +481,30 @@ def test_get_answers__unknown_interaction_provided(): @pytest.mark.parametrize( - ["basic_question_should_ask", "expected_basic_question_ask_call_count"], - [(True, 1), (False, 0)], + [ + "basic_question_should_ask", + "expected_basic_question_ask_call_count", + "echo_should_ask", + "expected_echo_display_call_count", + ], + [(True, 1, False, 0), (False, 0, True, 1)], ) def test_get_answers__proper_funcs_called( - mocker, basic_question_should_ask, expected_basic_question_ask_call_count + mocker, + basic_question_should_ask, + expected_basic_question_ask_call_count, + echo_should_ask, + expected_echo_display_call_count, ): """ Given a list of interactions of different types, validate the proper funcs should be invoked on each - The parametrization asserts that Question interactions are only 'asked' when they should be asked + The parametrization asserts that Question interactions are only 'asked' when they should be asked and + that Displayable interactions are only 'displayed' when they should be displayed. """ - echo_interaction_mock = mocker.Mock(spec=Echo) + echo_interaction_mock = mocker.Mock( + spec=Echo, should_ask=mocker.Mock(return_value=echo_should_ask) + ) basic_question_interaction_mock = mocker.Mock( spec=BasicQuestion, should_ask=mocker.Mock(return_value=basic_question_should_ask), @@ -486,7 +518,9 @@ def test_get_answers__proper_funcs_called( # functions belonging to interactions should be invoked # in the case of a question, it either should or shouldn't be asked - echo_interaction_mock.display.assert_called_once() + # in the case of echo, it either should or shouldn't be displayed + echo_interaction_mock.should_ask.assert_called_once() + assert echo_interaction_mock.display.call_count == expected_echo_display_call_count basic_question_interaction_mock.should_ask.assert_called_once() assert ( basic_question_interaction_mock.ask.call_count diff --git a/tests/sample_data.py b/tests/sample_data.py index 7003c814..93bbb3e3 100644 --- a/tests/sample_data.py +++ b/tests/sample_data.py @@ -4,6 +4,7 @@ BasicQuestion, Choice, Confirm, + Displayable, Question, canonical_arg_name, ) @@ -53,6 +54,21 @@ def ask(self, answers, no_user_input=False): raise Exception("Don't call") +class SampleDisplayable(Displayable): + """Displayable for testing base class functionality""" + + def __init__(self, message, should_ask=None): + super().__init__(message, should_ask) + self._display_called = False + + def display(self, answers): + self._display_called = True + + @property + def display_called(self) -> bool: + return self._display_called + + # combination of questions that reuse the same name DUPLICATE_QUESTION_NAME_PARAMS = [ [