Merge pull request #358 from wayfair-incubator/feat/should-ask-for-ec…

…ho-and-ack

feat: `ShouldAsk` for `Echo` and `Acknowledge`

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
 

columbo/__init__.py

@@ -10,6 +10,7 @@
     BasicQuestion,
     Choice,
     Confirm,
+    Displayable,
     Echo,
     Interaction,
     Question,

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)

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,

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?",

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?",

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
 

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
 

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!}

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