-
-
Notifications
You must be signed in to change notification settings - Fork 33
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
Fix #782: give implementations more flexibility in error handling #795
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "in all cases" text with the "MUST or MUST" is a bit unusual in terms of standards. Perhaps:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK. I am OK with the suggestion, and I trust you on what standards do :-)
But isn't the final form a bit too mild?
The result is "can A or B or both"
Does it leave an open door to do neither A nor B?
Would something like this be more in line with what standards do?: "MUST A or B or both"
Which would preserve (at least some) of the "strength" of the original text?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@macchiati I think maybe a better pseudo-model would look like the below. This ignores the try/catch pattern, which would look like your example, to show some of the other ways APIs might be provided:
@eemeli is correct that any of the three models he lists would be permitted by the proposed text. I think I agree that we should make a design doc, even if it is a small one, since I note that some programming languages (such as Java) use exception throwing as the primary error mechanism. But some do not. We need to enable those languages also.
I think it is probably incorrect to only have a fallback message with no way to find out if the formatting call failed in some way.
So I'd suggest something like:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be okay with any of: 1) @mihnita's proposed change 2) @aphillips's proposed change so long as the change below is made 3) just changing the existing spec text's "MUST"s to "MAY"s, or 4) some combination thereof.
The change to @aphillips's proposed change above is:
I think that change supports the constraints of something like ICU4X in Rust where it only reports 1 error if there are many errors encountered, due to the constraint that they don't want to allocate but allocation would be required if they had to report any & all errors, according to the feedback that we asked them to give. And I think it better reflects @aphillips's original intent when he wrote above
I say this also because the hypothetical Java-ish pseudocode above also includes the line
which implies getting multiple errors, but that's probably unnecessary/undesirable for many places in which Rust is used, and is not supported by ICU4X in particular.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR, plus maybe at least this bit of editing, is an improvement over the original. One key thing to note is that the most idiomatic way to implement in many languages will be more than one formatter function. For example in rust where you can return a
Result<message, error>
, where you only get either message or error but not both, providing "a means for the caller to obtain some representation" will look likeThe usage would be to try with
message.format()
first using normal idiomatic error handling to determine if it worked or not. In the event an error was returned instead of a formatted string, then the programmer has the option to use the infallible formatFallback variant that does whatever it needs to do to return something (even if it is just the message source or even the id) but is unable to return any error info at the same time.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be fine with a change that separated the two current MUST statements from each other to more explicitly allow an implementation like @alerque's above to be conformant.
As I see it, the two key requirements we are and should be imposing are:
Note in particular the use of the word "valid" there; that spec-defined qualifier is intentional, as we probably should not require messages with syntax or data model errors to still be nominally formattable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eemeli noted about valid messages. It should be pointed out that invalid messages would fail before one starts the formatting process. (Of course it is possible to visualize a method/function that parses and formats a message all in one go, but under the hood the implementation will fail the message before it gets to the formatting steps in our spec)
Note that the fallback representation of syntax and data model errors (these errors define what it means to be a "valid" message) is the logo string, specifically:
@echeran noted:
I would probably make a change to your proposal. An implementation that doesn't want to return a structure containing all of the errors might reasonably be "fast fail" (stop trying to format the message and return the first error it encounters rather than trying to "collect them all": it's not clear that an implementation can always recover to find all of the errors anyway or that removing an error might not result in a different outcome).
@alerque I agree that separate functions are one possibility (another example that somewhere I call out is ICU4J sometimes has "throwing" and "non-throwing" format methods).
Note that
fn format($self, args: Arguments) -> Result<String,Error>;
could return both String and Error for every call, withError::NONE
being the "good result", saving the expense of formatting twice. When there is an error, the string can be the fallback string. I don't think we should require this behavior. Just calling it out.As a user, I'd be unamused by the need to write tons of logic around my format calls. One of the nice things about message format is the ability to get rid of tons of fiddly code related to the arguments and their formatting. I tend to want my calls to MF2 to be as sleek as possible. In most cases I'm going to try to solve all of my errors in testing, so I can be "free to fly" in production.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it can't, that's impossible. Rust results are either a successful something (type specified) or an error, but never both.
The only way around that is to re-roll your own error handling from scratch by making your own types (with a tuple struct or whatever) with results and errors inside them—and eschewing all of Rust's error handling patterns. And that's crazy. This is the whole reason this issue came up because the way the spec is written would force people to write non-ergonomic and very much non-idiomatic code in many languages just to be spec compliant.
That's exactly what an end user would have to do in many languages if the API followed the patterns the spec is currently trying to mandate and/or keep getting suggested here as possible patterns. Maybe in some languages that is an ergonomic/idiomatic possibility, but not Rust and not many other languages I know of. The message format shouldn't be mandating this aspect of an implementation API. It can make suggestions so that robust usage is possible, but shouldn't try to foist a particular error handling pattern onto languages that may or may not actually work that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alerque Never mind then. I have no desire to create special work for Rust implementers (the whole point of my suggestions is to allow idiomatic implementation everywhere)