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.
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
Add Date, Time, and DateTime from str impls via IXDTF #5260
Add Date, Time, and DateTime from str impls via IXDTF #5260
Changes from 18 commits
e6c0e2d
59b40a7
e2a012e
70047f5
4b361d1
05f5fa9
de60458
97663dd
de0a56e
5594033
444c064
b937e49
2d9a022
e8b63bb
ce2ba13
e3c9962
15543e8
504d026
1f5b4c9
87299ff
9d7e12a
32eac8f
8800afd
e47fe2a
a84b333
d51d245
bb88ea6
78a6947
49c4152
0eb45b1
3f4c0ac
d54fa6e
78df9bf
c3d8f08
752151f
6ae5f22
67f6e9c
f87d82e
cebaca9
8bf9e89
9303143
b607aca
cfc0b1a
b851b04
db13889
f9c04f8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Thinking about it, I'd call this
Syntax
as well.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.
these are also public API, might want to do
map_err(FromIxdtfError::Ixdtf)
insteadThere 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 thought about that but they are unavoidably in the public API as being a variant of
FromIxdtfError
(nowFromStrError
)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.
A few choices here:
ixdtf::ParserError
stays semver compatibleSyntax
with no details in the variantixdtf
crate reaches stable by the time we release the nexticu_calendar
crate versionCC @Manishearth @nekevss
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.
Define "stable"? The crate works and has a reasonable API right now, no?
These impls simplify some code in this file (barely), which I don't think is worth the cost of them showing up in docs, regardless of how stable ixdtf is.
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.
"stable" meaning version 1.0 of the
ixdtf
crate.I don't quite understand the harm in having From impls when they are truly trivial, wrapping the error with a specific enum variant.
From impls barely show up in docs because they are a standard library trait implementation.
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 don't understand that requirement,
fixed_decimal
,litemap
,tinystr
,writeable
, are all crates that appear on our APIs and are pre-1.0.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 realized I don't really like option 2, either, because I'd want to add the data when we can, and adding a value to the
Syntax
variant is also itself a semver change.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.
hmm the records feel like ixdtf parser implementation types, so I'd rather not expose conversions from them.
Date
andTime
are trivial enough to be done manually, and forAnyCalendar
I don't really see the use caseThere 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 figured since
DateRecord
andTimeRecord
are public types, we should have public conversions. But, actually, it putsixdtf
intoicu_datetime
's API, so we should probably at least wait untilixdtf
has a 1.0 release.