Add RelativeTimeFormatter #2846
Conversation
| plural_rules_mapping | ||
| .other | ||
| .as_ref() | ||
| .expect("Mapping for PluralCategory::Other must be present.") |
There was a problem hiding this comment.
Issue: .expect is too strong here. This is a good example of something you should check at deserialization time. See #2768
There was a problem hiding this comment.
Making other a required field in relativetime/src/provider.rs.
This removes the need for expect.
| sink.with_part(parts::LITERAL, |s| s.write_str(&pattern.pattern))?; | ||
| } else { | ||
| sink.with_part(parts::LITERAL, |s| { | ||
| s.write_str(&pattern.pattern[..pattern.index as usize]) |
There was a problem hiding this comment.
Replaced this by split_at function.
- Fix negative number checking - Move lib test to doc test - Fix docs in relativetime/src/error.rs - Change format method to take Into<FixedDecimal>
- Handle Numeric::Auto - Add doc tests
| } else { | ||
| let (prefix, suffix) = singular_sub_pattern | ||
| .pattern | ||
| .split_at(singular_sub_pattern.index as usize); |
There was a problem hiding this comment.
Issue: split_at is panicky. It would be nice if there were a lint to catch it. Please see https://github.com/unicode-org/icu4x/pull/2854/files for a workaround
There was a problem hiding this comment.
Is it okay to use split_at with explicitly checking singular_sub_pattern.index <= pattern.len() ?
There was a problem hiding this comment.
It is necessary to check the length before calling split_at.
There was a problem hiding this comment.
Ah yes. Using code similar to the above PR.
| .split_at(singular_sub_pattern.index as usize); | ||
|
|
||
| sink.with_part(parts::LITERAL, |s| s.write_str(prefix))?; | ||
| // TODO: Remove this clone. |
There was a problem hiding this comment.
Suggestion: when constructing the FormattedRelativeTime, store a bool for whether it is negative or not and remove the sign at that point.
- Remove displaynames component from deps - Make RelativeTimeFormatter.format take FixedDecimal arg - Fix clippy error
| fn write_to_parts<S: writeable::PartsWrite + ?Sized>(&self, sink: &mut S) -> core::fmt::Result { | ||
| if self.options.numeric == Numeric::Auto { | ||
| let relatives = &self.formatter.rt.get().relatives; | ||
| if let Ok(i8_value) = self.value.to_string().parse::<i8>() { |
There was a problem hiding this comment.
Just noticed this: we shouldn't convert to a string and parse here. Better would be something along the lines of
if self.value.magnitude_range() == 0..=0 {
let u8_value = self.value.digit_at(0);
// ...
}
Also, note that this doesn't currently work correctly with negative numbers since we remove the sign earlier now.
There was a problem hiding this comment.
The values are either -2, -1, 0, 1, 2 in the data. The 0th digit + sign is sufficient.
|
Please update missing_apis.txt |
|
@pdogr I think you can ignore the Coverage CI failure and merge this. |
3 participants
#802