Issue 150 #151
Issue 150 #151
Conversation
…tions are wrapped in `Silently`
| @@ -49,6 +49,7 @@ def Silently(duck: T) -> T: | |||
| ) | |||
| """ | |||
| if settings.UNABRIDGED_NARRATION: | |||
| duck._silenced = False # type: ignore[attr-defined] | |||
There was a problem hiding this comment.
issue: I don't think this is necessary, and may actually cause a problem if the narration is only temporarily unabridged (like Loudly's implementation (which i just realized we never actually implemented—i'll get on that soon!)).
There was a problem hiding this comment.
In that case wouldn't Loudly purposefully set False?
My thinking here is by setting the attribute at all, it marks the object as having been wrapped in Silently which is similar but distinct from whether or not it was actually made to be silent (e.g. UNABRIDGED was on vs off).
I just wasn't sure if that was something we might want.
screenpy/actions/silently.py
Outdated
| def is_silent(duck: Performable | Resolvable | Answerable) -> bool: | ||
| """Check if a duck has been silenced.""" | ||
| if isinstance(duck, Silenced): | ||
| return duck._silenced | ||
| return False |
There was a problem hiding this comment.
suggestion: I think this might belong in speech_tools.py instead of here. What do you think?
It just doesn't feel right to import is_silent from an Action file.
There was a problem hiding this comment.
I agree. Speech tools didn't feel right either. Maybe we need a functions module? I hesitate to make something like "utils".
screenpy/protocols.py
Outdated
|
|
||
|
|
||
| @runtime_checkable | ||
| class Silenced(Protocol): |
There was a problem hiding this comment.
question: Do we want to keep the -able postfix we've got for most of these? Unspeakable sounds kinda fun. :P
There was a problem hiding this comment.
I thought about that too. I second guessed the use of Silenceable due to possible confusion between the distinction of "having been wrapped in Silently" vs is_silent.
Am I over thinking it or is there possibly another term that would describe without confusion?
There was a problem hiding this comment.
The more I think about it.. I like the -able since it also helps drive the point that wrapped classes aren't strictly silent without knowing what UNABRIDGED_NARRATION was set to.
|
|
||
| t = Either(mock_action1, Silently(mock_action2)).or_( | ||
| mock_action3, Silently(mock_action4) | ||
| ) |
There was a problem hiding this comment.
nitpick: The nitpickiest of nitpicks, can you add a blank line after this one to separate the Act and Assert steps?
…t why it complains about the docstring formatting
addresses #150