-
Notifications
You must be signed in to change notification settings - Fork 101
Update FluentBundle to the latest API #120
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
Merged
Merged
Changes from all commits
Commits
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 hidden or 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 hidden or 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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,20 +38,23 @@ fn get_ids(res: &FluentResource) -> Vec<String> { | |
.collect() | ||
} | ||
|
||
fn get_args(name: &str) -> Option<HashMap<&'static str, FluentValue>> { | ||
fn get_args(name: &str) -> Option<HashMap<String, FluentValue>> { | ||
match name { | ||
"preferences" => { | ||
let mut prefs_args = HashMap::new(); | ||
prefs_args.insert("name", FluentValue::from("John")); | ||
prefs_args.insert("tabCount", FluentValue::from(5)); | ||
prefs_args.insert("count", FluentValue::from(3)); | ||
prefs_args.insert("version", FluentValue::from("65.0")); | ||
prefs_args.insert("path", FluentValue::from("/tmp")); | ||
prefs_args.insert("num", FluentValue::from(4)); | ||
prefs_args.insert("email", FluentValue::from("[email protected]")); | ||
prefs_args.insert("value", FluentValue::from(4.5)); | ||
prefs_args.insert("unit", FluentValue::from("mb")); | ||
prefs_args.insert("service-name", FluentValue::from("Mozilla Disk")); | ||
prefs_args.insert("name".to_string(), FluentValue::from("John")); | ||
prefs_args.insert("tabCount".to_string(), FluentValue::from(5)); | ||
prefs_args.insert("count".to_string(), FluentValue::from(3)); | ||
prefs_args.insert("version".to_string(), FluentValue::from("65.0")); | ||
prefs_args.insert("path".to_string(), FluentValue::from("/tmp")); | ||
prefs_args.insert("num".to_string(), FluentValue::from(4)); | ||
prefs_args.insert("email".to_string(), FluentValue::from("[email protected]")); | ||
prefs_args.insert("value".to_string(), FluentValue::from(4.5)); | ||
prefs_args.insert("unit".to_string(), FluentValue::from("mb")); | ||
prefs_args.insert( | ||
"service-name".to_string(), | ||
FluentValue::from("Mozilla Disk"), | ||
); | ||
Some(prefs_args) | ||
} | ||
_ => None, | ||
|
@@ -91,7 +94,14 @@ fn resolver_bench(c: &mut Criterion) { | |
|
||
b.iter(|| { | ||
for id in &ids { | ||
let (_msg, errors) = bundle.compound(id, args.as_ref()).expect("Message found"); | ||
let msg = bundle.get_message(id).expect("Message found"); | ||
let mut errors = vec![]; | ||
if let Some(value) = msg.value { | ||
let _ = bundle.format_pattern(value, args.as_ref(), &mut errors); | ||
} | ||
for (_, value) in msg.attributes { | ||
let _ = bundle.format_pattern(value, args.as_ref(), &mut errors); | ||
} | ||
assert!(errors.len() == 0, "Resolver errors: {:#?}", errors); | ||
} | ||
}) | ||
|
This file contains hidden or 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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.
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.
It would be nicer to use
if let Some(value) = msg.value { .. }
guard around the call toformat_pattern
in all examples, because it's much closer to a real-world usage of the API.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'm struggling with that a bit. I know this is a low level API that we expect to build on top of, but if someone would use it in their app, they wouldn't branch for a scenario where the string is missing, they'd expect the value of a string to be there if they placed it in their FTL file.
I don't want to suggest to localize an app by replacing:
with:
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.
This only works if their FTL file is the only FTL file in the app. As soon as they have more FTLs for more locales, this assumption may not hold anymore, depending on who wrote them. As you know, Fluent is designed around the idea that translations must not break the app. Handling that
Option<Pattern>
is an important enforcement of this design.I know this is an example, and it's OK to simplify. But there is a chance people will look at examples to make their first steps around Fluent, and I'd like to take the opportunity to educate :) There's also a chance people will copy&paste directly from examples.
Also, it's approximately the same amount of code, and some might argue that the
if let
is actually nicer.Or, to make the null-value case explicit, I'd put a comment in the
else
block, like so: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.
Yes, but we achieve it on a higher level. We introduce nice fallback mechanisms that handle the case where the user expects a value and there is none.
On this level there is no such mechanism, no such fallback.
I believe for a reader of an example, seeing:
is bizarre. It's basically saying "try to localize the message, and if that fails, make your app show nothing at all".
I believe that
.expect()
is exactly the right call for the example. It says "Since you placed the value, expect it to be there, but please, remember that it's anOption
and it may be null, so you'll likely have a macro/higher-level-api that will facilitate a fallback".This is more acceptable, but still kind of explicit. It gives the vibe of "Fluent makes your code way less readable.".
I'd prefer to use
.expect
to indicate that we expect the message, and I intend to improve docs to clearly state that the purpose of FluentBundle is to be handled by a higher level abstraction that will handle fallbacking, and since fallbacking may differ between bindings and apps, we make FluentBundle API clean for the higher level API to pick their strategy.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.
OK, that sounds fair, thanks!