Repository: projectfluent/fluent-rs (rust)

Issue:
Add support for `FluentBundle::add_resource` allowOverrides
JS implementation allows to set `allowOverrides` to true and allow for new resources to override messages in bundle, see https://github.com/projectfluent/fluent.js/pull/332

Pull request:
Add support for adding resources with overrides
Fixes #142.

Stas, I'd like to get your r? mostly for the API decisions, in an effort to bring fluent-rs more into Fluent core realm.

The JS API relies heavily on default argument values for API ergonomics and in Rust we don't have the same luxury.

There are three options I can see using:

1) Required arguments:

```rust
impl FluentBundle {
    fn add_resource(&mut self, r: R, allow_overrides: bool) -> Result<()>;
}
```

2) Separate API:

```rust
impl FluentBundle {
    fn add_resource(&mut self, r: R, allow_overrides: bool) -> Result<()>;
    fn add_resource_with_overrides(&mut self, r: R);
}
```

3) Options bag with defaults:

```rust
#[derive(Default)]
struct FluentBundleAddResourceOptions {
    allow_overrides: bool
}

impl FluentBundle {
    fn add_resource(&mut self, r: R, options: FluentBundleAddResourceOptions) -> Result<()>;
}
```

allowing for:

```rust
bundle.add_resource(&res, FluentBundleAddResourceOptions::default());
```

or maybe even `Option<FluentBundleAddResourceOptions>` to allow for `add_resource(&res, None)`

or maybe even:

```rust
impl FluentBundle {
    fn add_resource(&mut self, r: R, options: FluentBundleAddResourcOptions) -> Result<()>;
    fn add_resource_with_defaults(&mut self, r: R) -> Result<()>;
}
```

========

My current approach, which is based on observation of how stdlib APIs are designed, but not really confirmed and may be just my interpretation is that I want to use (1) very rarely and only in cases where I believe the decision should be consciously made each and every time the API is used.
I use (2) for most cases where the defaults are sane and I expect them to be used most of the time, but I want to allow for alternative behavior.

I have never seen anyone doing (3) but it may be because we are still fairly early in the high-level APIs design, so maybe no standard model emerged yet.

One nice thing about (2) in *this particular* case is that it allows me to drop `Result` because the method becomes infallible, but for general design decisions that shouldn't be a main factor.

Patch:
diff --git a/fluent-bundle/src/bundle.rs b/fluent-bundle/src/bundle.rs
index 8555d7e..f4fb03e 100644
--- a/fluent-bundle/src/bundle.rs
+++ b/fluent-bundle/src/bundle.rs
@@ -181,6 +181,10 @@ impl<R> FluentBundle<R> {
 
     /// Adds a resource to the bundle, returning an empty [`Result<T>`] on success.
     ///
+    /// If any entry in the resource uses the same identifier as an already
+    /// existing key in the bundle, the new entry will be ignored and a
+    /// `FluentError::Overriding` will be added to the result.
+    ///
     /// The method can take any type that can be borrowed to FluentResource:
     ///   - FluentResource
     ///   - &FluentResource
@@ -267,6 +271,94 @@ impl<R> FluentBundle<R> {
         }
     }
 
+    /// Adds a resource to the bundle, returning an empty [`Result<T>`] on success.
+    ///
+    /// If any entry in the resource uses the same identifier as an already
+    /// existing key in the bundle, the entry will override the previous one.
+    ///
+    /// The method can take any type that can be borrowed as FluentResource:
+    ///   - FluentResource
+    ///   - &FluentResource
+    ///   - Rc<FluentResource>
+    ///   - Arc<FluentResurce>
+    ///
+    /// This allows the user to introduce custom resource management and share
+    /// resources between instances of `FluentBundle`.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use fluent_bundle::{FluentBundle, FluentResource};
+    /// use unic_langid::langid;
+    ///
+    /// let ftl_string = String::from("
+    /// hello = Hi!
+    /// goodbye = Bye!
+    /// ");
+    /// let resource = FluentResource::try_new(ftl_string)
+    ///     .expect("Could not parse an FTL string.");
+    ///
+    /// let ftl_string = String::from("
+    /// hello = Another Hi!
+    /// ");
+    /// let resource2 = FluentResource::try_new(ftl_string)
+    ///     .expect("Could not parse an FTL string.");
+    ///
+    /// let langid_en = langid!("en-US");
+    ///
+    /// let mut bundle = FluentBundle::new(&[langid_en]);
+    /// bundle.add_resource(resource)
+    ///     .expect("Failed to add FTL resources to the bundle.");
+    ///
+    /// bundle.add_resource_overriding(resource2);
+    ///
+    /// let mut errors = vec![];
+    /// let msg = bundle.get_message("hello")
+    ///     .expect("Failed to retrieve the message");
+    /// let value = msg.value.expect("Failed to retrieve the value of the message");
+    /// assert_eq!(bundle.format_pattern(value, None, &mut errors), "Another Hi!");
+    /// ```
+    ///
+    /// # Whitespace
+    ///
+    /// Message ids must have no leading whitespace. Message values that span
+    /// multiple lines must have leading whitespace on all but the first line. These
+    /// are standard FTL syntax rules that may prove a bit troublesome in source
+    /// code formatting. The [`indoc!`] crate can help with stripping extra indentation
+    /// if you wish to indent your entire message.
+    ///
+    /// [FTL syntax]: https://projectfluent.org/fluent/guide/
+    /// [`indoc!`]: https://github.com/dtolnay/indoc
+    /// [`Result<T>`]: https://doc.rust-lang.org/std/result/enum.Result.html
+    pub fn add_resource_overriding(&mut self, r: R)
+    where
+        R: Borrow<FluentResource>,
+    {
+        let res = r.borrow();
+        let res_pos = self.resources.len();
+
+        for (entry_pos, entry) in res.ast().body.iter().enumerate() {
+            let id = match entry {
+                ast::ResourceEntry::Entry(ast::Entry::Message(ast::Message { ref id, .. }))
+                | ast::ResourceEntry::Entry(ast::Entry::Term(ast::Term { ref id, .. })) => id.name,
+                _ => continue,
+            };
+
+            let entry = match entry {
+                ast::ResourceEntry::Entry(ast::Entry::Message(..)) => {
+                    Entry::Message([res_pos, entry_pos])
+                }
+                ast::ResourceEntry::Entry(ast::Entry::Term(..)) => {
+                    Entry::Term([res_pos, entry_pos])
+                }
+                _ => continue,
+            };
+
+            self.entries.insert(id.to_string(), entry);
+        }
+        self.resources.push(r);
+    }
+
     /// When formatting patterns, `FluentBundle` inserts
     /// Unicode Directionality Isolation Marks to indicate
     /// that the direction of a placeable may differ from


Test patch:
diff --git a/fluent-bundle/tests/bundle.rs b/fluent-bundle/tests/bundle.rs
new file mode 100644
index 0000000..eb0cee1
--- /dev/null
+++ b/fluent-bundle/tests/bundle.rs
@@ -0,0 +1,37 @@
+use fluent_bundle::{FluentBundle, FluentResource};
+use unic_langid::LanguageIdentifier;
+
+#[test]
+fn add_resource_override() {
+    let res = FluentResource::try_new("key = Value".to_string()).unwrap();
+    let res2 = FluentResource::try_new("key = Value 2".to_string()).unwrap();
+
+    let en_us: LanguageIdentifier = "en-US"
+        .parse()
+        .expect("Failed to parse a language identifier");
+    let mut bundle = FluentBundle::<&FluentResource>::new(&[en_us]);
+
+    bundle.add_resource(&res).expect("Failed to add a resource");
+
+    assert!(bundle.add_resource(&res2).is_err());
+
+    let mut errors = vec![];
+
+    let value = bundle
+        .get_message("key")
+        .expect("Failed to retireve a message")
+        .value
+        .expect("Failed to retireve a value of a message");
+    assert_eq!(bundle.format_pattern(value, None, &mut errors), "Value");
+
+    bundle.add_resource_overriding(&res2);
+
+    let value = bundle
+        .get_message("key")
+        .expect("Failed to retireve a message")
+        .value
+        .expect("Failed to retireve a value of a message");
+    assert_eq!(bundle.format_pattern(value, None, &mut errors), "Value 2");
+
+    assert!(errors.is_empty());
+}