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

Stephen's non-nits #381

Closed
wants to merge 4 commits into from
Closed

Stephen's non-nits #381

wants to merge 4 commits into from

Conversation

gselander
Copy link
Collaborator

@gselander
Copy link
Collaborator Author

This is a PR on top of the Stephen's-nits PR. The recent commit addressed this non-nit:

The intent here and all other message processing is that you carry out the processing in steps and if any step fail then you don't carry out the subsequent steps. Is that clear enough?

I'm not sure it's clear enough - esp wrt passing on EAD which can have side-effects that could be bad (e.g. if some identifier were stored even though message processing had failed for some reason). I think the EAD stuff is the only way that kind of thing might happen - if so, then maybe saying that EAD should only be passed to applications after all processing steps have succeeded might be right?

@gselander gselander mentioned this pull request Dec 9, 2022
@gselander
Copy link
Collaborator Author

@sftcd Please check 7631715

@emanjon
Copy link
Collaborator

emanjon commented Dec 17, 2022

This is hard to review due to the overlap with the nits PR. The nits PR seems ready to merge, this is not.

@emanjon
Copy link
Collaborator

emanjon commented Dec 19, 2022

I looked at the commits 89b46cc and 5d6913d

They look good and ready to merge

@gselander
Copy link
Collaborator Author

Closing this in favor of #383

@gselander gselander closed this Dec 19, 2022
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.

None yet

2 participants