Update FluentBundle to the latest API #120

zbraniecki · 2019-07-12T23:08:56Z

Implementes #119 119

zbraniecki · 2019-07-12T23:49:53Z

I kept the convenience methods temporarily until I update the tests.

zbraniecki · 2019-07-23T03:18:21Z

@stasm - I don't want to overload you as I know you're super-busy, so I'm not going to block on your review, but I think this patchset is ready and it puts us roughly on par with the JS.

There are still some areas missing (mainly lack of NUMBER/DATETIME and bomb protection), but I'd prefer to land this patch and continue catching up to JS functionality on master.

zbraniecki · 2019-07-23T03:20:06Z

One change I placed here is to make l10n_args use owned strings as keys. &str as key sucks more often than not, and I filed #123 to come up with a clean solution that has better ergonomics (maybe Cow?). For now, owned keys should work without too much hassle.

stasm · 2019-07-23T14:41:14Z

fluent-bundle/examples/external_arguments.rs

-    }
+    let msg = bundle.get_message("hello-world").expect("Message exists");
+    let mut errors = vec![];
+    let pattern = msg.value.expect("Message has a value");


expect() takes the description of the error scenario, rather than of the success one :)

stasm · 2019-07-23T14:43:34Z

fluent-bundle/examples/external_arguments.rs

+    let msg = bundle.get_message("hello-world").expect("Message exists");
+    let mut errors = vec![];
+    let pattern = msg.value.expect("Message has a value");
+    let value = bundle.format_pattern(&pattern, Some(&args), &mut errors);


It would be nicer to use if let Some(value) = msg.value { .. } guard around the call to format_pattern in all examples, because it's much closer to a real-world usage of the API.

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:

println!("Enter your age:");

with:

if let Some(value) = msg.value { println!(bundle.format_pattern(value, None)); } else { unimplemented!() }

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.

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.

let pattern = msg.value.expect("Message has no value"); println!(bundle.format_pattern(&pattern, None));

if let Some(value) = msg.value { println!(bundle.format_pattern(value, None)); }

Or, to make the null-value case explicit, I'd put a comment in the else block, like so:

if let Some(value) = msg.value { println!(bundle.format_pattern(value, None)); } else { // Show the fallback translation, e.g. the id of the message. }

As you know, Fluent is designed around the idea that translations must not break the app.

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:

if let Some(value) = msg.value { println!(bundle.format_pattern(value, None)); }

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 an Option and it may be null, so you'll likely have a macro/higher-level-api that will facilitate a fallback".

if let Some(value) = msg.value { println!(bundle.format_pattern(value, None)); } else { // Show the fallback translation, e.g. the id of the message. }

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.

OK, that sounds fair, thanks!

stasm · 2019-07-23T14:44:46Z

fluent-bundle/src/bundle.rs

@@ -21,6 +21,12 @@ use crate::types::FluentValue;

 #[derive(Debug, PartialEq)]
 pub struct Message<'m> {


Would it make sense to call this RawMessage?

I don't think so. We have FluentBundle::get_message, so we return a Message. There's no other Message in the crate.

how about FluentMessage, then? I'm concerned about the naming conflict with ast::Message. I understand that it's not really a problem in code, because the module namespacing works to our advantage. But conceptually, I'd prefer for these two "messages" to have distinct names.

Yep, filed an issue!

stasm · 2019-07-23T14:45:49Z

fluent-bundle/src/bundle.rs

-///     .expect("Failed to format a message.");
+/// let msg = bundle.get_message("intro").expect("Message exists");
+/// let mut errors = vec![];
+/// let pattern = msg.value.expect("Message has a value");


Same as in examples, please use a proper if let Some(value) = ....

Also, I can't comment on the exact line, but this docstring mentions format and compound a few times.

stasm · 2019-07-23T14:52:31Z

fluent-bundle/src/bundle.rs

+        pattern: &'bundle ast::Pattern,
+        args: Option<&'bundle HashMap<String, FluentValue>>,
+        errors: &mut Vec<FluentError>,
+    ) -> Cow<'bundle, str>


Are you still planning to change the return value of format_pattern to (String, Option<Vec<Error>>)?

Yeah, I'll look into this after I land this patch to verify the performance impact.

stasm · 2019-07-23T14:53:25Z

fluent-bundle/src/bundle.rs

+        for attr in message.attributes.iter() {
+            attributes.insert(attr.id.name, &attr.value);
+        }
+        return Some(Message { value, attributes });


Would it make more sense to do all this prep work in add_resource instead?

I'd prefer not to, at least for now.

zbraniecki self-assigned this Jul 12, 2019

zbraniecki force-pushed the bundle-api branch from 3d4875b to df42d43 Compare July 12, 2019 23:32

stasm mentioned this pull request Jul 15, 2019

Align FluentBundle with the new JS FluentBundle API #119

Closed

zbraniecki force-pushed the bundle-api branch 6 times, most recently from 42aaa05 to e2d1c09 Compare July 21, 2019 07:07

zbraniecki mentioned this pull request Jul 21, 2019

Minor fixes on simple-app example + extra translation #124

Merged

zbraniecki force-pushed the bundle-api branch 4 times, most recently from daa1766 to cfda316 Compare July 23, 2019 02:43

zbraniecki marked this pull request as ready for review July 23, 2019 03:16

stasm mentioned this pull request Jul 23, 2019

Refactor FluentArgs for better ergonomics #123

Closed

stasm reviewed Jul 23, 2019

View reviewed changes

zbraniecki force-pushed the bundle-api branch 2 times, most recently from ae523c2 to 962196d Compare July 23, 2019 21:48

Update FluentBundle to the latest API

fc645cc

zbraniecki force-pushed the bundle-api branch from 962196d to fc645cc Compare July 23, 2019 21:57

zbraniecki mentioned this pull request Jul 23, 2019

Consider switching format_pattern to (String, Vec<Error>) #125

Closed

zbraniecki merged commit 2a4b92f into projectfluent:master Jul 24, 2019

zbraniecki mentioned this pull request Jul 25, 2019

Rename Message to FluentMessage #128

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update FluentBundle to the latest API #120

Update FluentBundle to the latest API #120

zbraniecki commented Jul 12, 2019

zbraniecki commented Jul 12, 2019

zbraniecki commented Jul 23, 2019

zbraniecki commented Jul 23, 2019

stasm Jul 23, 2019

stasm Jul 23, 2019

zbraniecki Jul 23, 2019

stasm Jul 24, 2019 •

edited

Loading

zbraniecki Jul 24, 2019 •

edited

Loading

stasm Jul 24, 2019

stasm Jul 23, 2019

zbraniecki Jul 24, 2019

stasm Jul 25, 2019 •

edited

Loading

zbraniecki Jul 25, 2019

stasm Jul 23, 2019

stasm Jul 23, 2019

stasm Jul 23, 2019

zbraniecki Jul 23, 2019

stasm Jul 23, 2019

zbraniecki Jul 23, 2019

		@@ -21,6 +21,12 @@ use crate::types::FluentValue;

		#[derive(Debug, PartialEq)]
		pub struct Message<'m> {

Update FluentBundle to the latest API #120

Update FluentBundle to the latest API #120

Conversation

zbraniecki commented Jul 12, 2019

zbraniecki commented Jul 12, 2019

zbraniecki commented Jul 23, 2019

zbraniecki commented Jul 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stasm Jul 24, 2019 • edited Loading

Choose a reason for hiding this comment

zbraniecki Jul 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stasm Jul 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stasm Jul 24, 2019 •

edited

Loading

zbraniecki Jul 24, 2019 •

edited

Loading

stasm Jul 25, 2019 •

edited

Loading