Conversation
Pull Request Test Coverage Report for Build 18106191436Details
💛 - Coveralls |
|
Scratch that. Past tense makes sense here because they're events that happened, not commands. Names make sense as-is. |
DanGould
left a comment
There was a problem hiding this comment.
As discussed offline, fn apply_... names still need updates
For example apply_maybe_inputs_owned must become apply_checked_broadcastable (Or really apply_checked_broadcast_suitability if we're keeping congruence with the check method names)
af66471 to
82d36dc
Compare
|
Changes since last review:
|
SessionEvent variants should describe "what event happened" instead of "what this event produces". This makes the inner values less ambiguous and better abides by the principle that events only capture primary information. We already do this for the `Created` and `Closed` variants. `SessionInvalid` was left as-is because it will be replaced soon.
Rename the transition methods according to the corresponding events.
82d36dc to
7586c53
Compare
Follow up to discussion in #1106 (comment). Note that I stuck with past tense over imperative mood because IMO that indicates more clearly that this event happen-ed (e.g.
RetrievedOriginalPayloadwould be difficult to express in imperative mood), also we already use past tense forCreatedandClosed.SessionEvent variants should describe "what event happened" instead of "what this event produces". This makes the inner values less ambiguous and better abides by the principle that events only capture primary information.
We already do this for the
CreatedandClosedvariants.SessionInvalidwas left as-is because it will be replaced soon.Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.