Implement era formatting #1346
Merged
Implement era formatting #1346
Conversation
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
Manishearth
requested review from
gregtatum,
nordzilla,
sffc,
zbraniecki and
a team
as code owners
sffc
requested changes
Nov 25, 2021
| 8 => Second::idx_in_range(&symbol), | ||
| 9 => TimeZone::idx_in_range(&symbol), | ||
| 0 => true, // eras | ||
| 1 => Year::idx_in_range(&symbol), |
There was a problem hiding this comment.
Comment: Changing these numbers would break the data file for older ICU4X versions. It's okay to make this change now, but after 1.0, we should add new items to the end, rather than changing the indices for all the old values.
There was a problem hiding this comment.
Yeah I'm trying to keep these in order; I think before 1.0 if we're missing symbols we should add them and just have them raise runtime errors
There was a problem hiding this comment.
(later) We should discuss options with Zibi. Perhaps we should use TinyStr here?
There was a problem hiding this comment.
Perhaps, but we won't be able to postprocess the TinyStr without allocating, so we'd have to use a string-matching ZeroMap, which isn't as cheap
sffc
reviewed
Nov 25, 2021
|
I think I'll move the code to using tinystr as a followup and simultaneously make the data structs borrow |
Manishearth
force-pushed
the
era-format
branch
from
7f19d25
to
061c48c
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
|
No code changes, just a rebase over the landed dep PR |
zbraniecki
previously approved these changes
Nov 29, 2021
| return symbols | ||
| .0 | ||
| .get(idx) | ||
| .map(|x| &**x) |
There was a problem hiding this comment.
optional (n-b): can you use .map(|x| x.as_ref()) here? It's more readable.
There was a problem hiding this comment.
It won't work since we're dereferencing a Cow here, deref coercions don't work through iterators like that
There was a problem hiding this comment.
Did you check? I pulled your branch, replaced with x.as_ref() and it compiles.
There was a problem hiding this comment.
.map(AsRef::as_ref) works as well
sffc
previously approved these changes
Nov 30, 2021
sffc
approved these changes
Nov 30, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Based on #1344
Fixes #1312
This is ready to review,
but still contains commits from #1344 (this PR starts at the commit labeled "add era field parsing" f75ac59)This implements DTF handling for the era symbol
Gand the ability to format eras.