Remove terminal_error method for sender and receiver#1123
Merged
arminsabouri merged 1 commit intopayjoin:masterfrom Oct 3, 2025
Merged
Remove terminal_error method for sender and receiver#1123arminsabouri merged 1 commit intopayjoin:masterfrom
arminsabouri merged 1 commit intopayjoin:masterfrom
Conversation
Collaborator
Pull Request Test Coverage Report for Build 18223941555Details
💛 - Coveralls |
7 tasks
Collaborator
|
Blocked by #1132 |
2 tasks
benalleng
force-pushed
the
remove-terminal-error-method
branch
3 times, most recently
from
49dda72 to
2b66380
Compare
benalleng
force-pushed
the
remove-terminal-error-method
branch
from
2b66380 to
a380dc0
Compare
arminsabouri
reviewed
Oct 2, 2025
benalleng
force-pushed
the
remove-terminal-error-method
branch
4 times, most recently
from
44d98bd to
47ebcd1
Compare
arminsabouri
reviewed
Oct 3, 2025
Collaborator
arminsabouri
left a comment
arminsabouri
left a comment
There was a problem hiding this comment.
Approach Ack 🔥
What else needs to be done for this to come out of draft?
Contributor
|
looks like squashing the commits, but that could be done w squash and merge |
Collaborator
Author
|
Sorry, stepped off before I marked it ready for review, I would like a definitive decision on whether to return only as an Ok resultand expect vs accepting errors as the 2 fixup commits take opposite approaches on the cli and test respectively, so deciding which to settle on before squashing them, but then this is ready to go. |
Closed
benalleng
force-pushed
the
remove-terminal-error-method
branch
from
47ebcd1 to
ef9c579
Compare
arminsabouri
reviewed
Oct 3, 2025
| .map(|e| e.to_json().to_string()), | ||
| }; | ||
| recv_rows.push(row); | ||
| match replay_receiver_event_log(&persister) { |
Collaborator
There was a problem hiding this comment.
Any reason we wouldnt match on a error for the active sessions? They can also expire and throw
Collaborator
Author
There was a problem hiding this comment.
Matched to errors on active sessions too now, but the completed_at field remains None on an error in these cases.
The public `terminal_error` method does not really serve a purpose as we now deal with errors directly in the state machine and don't really have an opportunity to call the `terminal_error` method where looking for an error wouldn't be sufficient.
benalleng
force-pushed
the
remove-terminal-error-method
branch
from
ef9c579 to
47bdef2
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The public
terminal_errormethod does not really serve a purpose as we now deal with errors directly in the state machine and don't really have an opportunity to call theterminal_errormethod where looking for an error wouldn't be sufficient.Closes #1118
Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.