Skip to content
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

feat: ShouldAsk for Echo and Acknowledge #358

Merged
merged 9 commits into from
Sep 2, 2022

Conversation

mklaber
Copy link
Collaborator

@mklaber mklaber commented Aug 26, 2022

  • Create an abstract class from which Echo and Acknowledge inherit
  • Add should_ask optional parameters
  • Only display a message if should_ask is None or True

Closes #356

* Create an abstract class from which `Echo` and `Acknowledge` inherit
* Add `should_ask` optional parameters
* Only display a message if `should_ask` is `None` or `True`
@mklaber mklaber requested a review from plannigan August 26, 2022 18:19
@codecov
Copy link

codecov bot commented Aug 26, 2022

Codecov Report

Merging #358 (4ede22c) into main (2b438a7) will increase coverage by 0.35%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #358      +/-   ##
==========================================
+ Coverage   96.82%   97.18%   +0.35%     
==========================================
  Files           6        6              
  Lines         347      355       +8     
  Branches       51       53       +2     
==========================================
+ Hits          336      345       +9     
+ Misses          9        8       -1     
  Partials        2        2              
Impacted Files Coverage Δ
columbo/__init__.py 100.00% <ø> (ø)
columbo/_interaction.py 100.00% <100.00%> (+0.65%) ⬆️

@mklaber mklaber marked this pull request as ready for review August 28, 2022 09:28
CHANGELOG.md Outdated Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
columbo/_interaction.py Show resolved Hide resolved
columbo/_interaction.py Show resolved Hide resolved
columbo/_interaction.py Outdated Show resolved Hide resolved
@@ -16,7 +19,8 @@ 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 "echo" that is only displayed
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
question. It also has an optional "echo" that is only displayed
question. It also has an optional `Echo` that is only displayed

Minor: I try to use the code formatting when referring to the classes by name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't referring to a class by name just as "two optional questions" wasn't referring to BasicQuestions. I used quotation marks because it is otherwise awkward to use echo as a noun here. I'll leave it as-is unless you have strong feelings (which it seems you don't).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Maybe something "It also has an optional message that is only...". That phrasing could remove the confusion I had.

@mklaber mklaber requested a review from plannigan August 28, 2022 14:13
CONTRIBUTING.md Outdated Show resolved Hide resolved
@@ -16,7 +19,8 @@ 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 "echo" that is only displayed
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Maybe something "It also has an optional message that is only...". That phrasing could remove the confusion I had.

@mklaber mklaber requested a review from plannigan August 29, 2022 13:22
Copy link
Owner

@plannigan plannigan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just one minor tweak to changelog.

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Patrick Lannigan <[email protected]>
@plannigan plannigan merged commit 4cc54eb into main Sep 2, 2022
@plannigan plannigan deleted the feat/should-ask-for-echo-and-ack branch September 2, 2022 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support should_ask in Echo interaction
2 participants