Resolve more pattern types into &str references #305
Conversation
|
Before these changes, Cow::Borrowed was returned 14 out of 174 times in fluent-bundle test cases. After the changes, I'm seeing 67 out of 174. |
|
@JasperDeSutter I didn't get a chance to finish reviewing all of your PRs before the holidays. I'll be back in the new year. |
|
I'm back from the holidays, but may still need a few days to get caught up for reviews. |
There was a problem hiding this comment.
Thanks for the PR! I am only marking as "Request Changes" so that I can see your response to my feedback. The PR looks good, I had a few minor comments and suggestions. Some of them are optional, which if you don't have the time, I can merge without them.
Please re-flag me for review, and we can get this merged!
| return variant.value.resolve(scope); | ||
| } | ||
| } | ||
| scope.add_error(ResolverError::MissingDefault); |
There was a problem hiding this comment.
Suggestion (optional): Is it possible to trigger this error in a test? It appears uncovered.
There was a problem hiding this comment.
This is unreachable code since a default variant is required, which is checked by the parser already. I think this doesn't warrant an explicit resolver error since we know it wont ever arise. A panic would be better suited IMO.
JasperDeSutter
force-pushed
the
more-patterns-by-ref
branch
from
5615698
to
7ad6779
Compare
alerque
force-pushed
the
more-patterns-by-ref
branch
from
98f63ba
to
908d6c7
Compare
|
Rebased to update commit messages, get current CI jobs to run, and run |
alerque
force-pushed
the
more-patterns-by-ref
branch
from
908d6c7
to
ae6fa28
Compare
alerque
force-pushed
the
more-patterns-by-ref
branch
from
ae6fa28
to
b3b2124
Compare
|
...and also fixed the |
| .map_or_else(|| value.into(), |transform| transform(value).into()); | ||
| } | ||
| ast::PatternElement::Placeable { expression } => { | ||
| let before = scope.placeables; |
There was a problem hiding this comment.
There's a ~7% performance regression in the resolve_to_str benchmark that I can't explain yet. It can be observed by commenting out the code in this match arm.
The most important change is in
fluent-bundle/src/resolver/pattern.rs, the rest is adding missing codepaths that were previously only done throughWriteValueand are now also needed inResolveValue.There are more cases where we can resolve to a borrowed str than just the TextElement case. For example there might be a select that resolves into a single TextElement, or a TermReference which is a simple string.
With these changes,
ResolveValuemethods will be called until a pattern with more than 1 element is found, which needs string concatenation and is handled by theWriteValuemethods as before.When resolving into a
FluentValue::Error, theWriteValueis called to get "{error}" formatting instead of an empty string.