-
Notifications
You must be signed in to change notification settings - Fork 536
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
Merge ParseError
with Error
, convert Parsed_set*
#1511
Open
pitdicker
wants to merge
3
commits into
chronotope:0.5.x
Choose a base branch
from
pitdicker:parse_rewrite_error
base: 0.5.x
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
As an extra note: the intention of this commit by merging
ParseError
intoError
is to make intermediate states possible.With this we can work on methods that currently return
ParseError
one by one without needing commits of many 100 lines and weird workarounds.The addition of the
Ambiguous
andInconsistent
variants are the only parts I do not intent to change 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.
Both of these error variants seem pretty specific to parsing, so I wonder if there's a different way of modeling these that would avoid having this in the top-level
Error
variant (since they couldn't occur in most of the places whereError
happens).Like, maybe
ParseError
gains anInvalid(Error)
variant?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.
Ambiguous
was mostly planned as an error variant that corresponds toMappedLocalTime::Ambiguous
, similar to howDoesNotExist
is planned to correspond to the currentMappedLocalTime::None
. It is nice thatAmbiguous
also makes sense for what is currentlyParseError::NotEnough
.Inconsistent
does not feel entirely unreasonable to have. I must admit its only use is with theParsed
type currently, which is of course closely related to parsing.Errors during parsing have an overlap with our general errors, only
InvalidArgument
would not be returned by it.Personally I see no problem in including three or four parsing-specific variants in our
Error
enum. I very much like to see them all in a single type with a single level (not nested) because that seems like the most convenient for our users.As I currently see it there is going to be one more variant in the
Error
enum after this to wrap the error type from #1448 (comment).Errors related to time zone conversion don't happen with the naive types, and only
OutOfRange
applies toTimeDelta
andFixedOffset
. Still it seems helpful to have them return the same type.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.
My point here is that there's a balance to strike here, and also there's some asymmetry here:
Error
variants can only be triggered by 10% of functions returning it, that doesn't seem great -- I'm not sure if the parsing-specific variants for chrono'sError
would be like that, but that would be my impression?Error
variants (because the alternative of introducing a large number of error types is worse)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.
What advantages do separate type for parse errors bring?
What advantages does it bring to have one error type?
All in all the advantages of either one or two errors are very tiny and theoretical. The types can have the same size (if we care, I do). Users can rarely if ever exhaustively match on the
Error
variants of any method, I'm not sure in the end we will have even one method than can return all variants. And we don't mark the enum as[non_exhaustive]
for nothing, users are not expected to do exhaustive matching.So it comes down to preference. Then my preference is for simplicity: one error type.
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.
Okay: let's do single error type for now, but I think we should reconsider splitting it before the 0.5.0 release.