Refactor FluentArgs for better ergonomics

[zbraniecki]

Currently FluentArgs is a HashMap over &str (and I'm going to change it to over String).

It would be nice to have it over Cow maybe and write some helper/macro to construct args.

[stasm]

From https://github.com/projectfluent/fluent-rs/pull/120#issuecomment-514037894:

&str as key sucks more often than not

Can you explain why that is? I'd assume using slices to static strings in args.insert("name", FluentValue::from("John")) would be the least controversial approach.

[zbraniecki]

Can you explain why that is? I'd assume using slices to static strings in args.insert("name", FluentValue::from("John")) would be the least controversial approach.

In real code you often work with programmatically generated arguments. Either directly, or passed from some external source.

In such cases, having to pass &str means that you have to allocate the ids somewhere earlier and keep the references to them for the lifetime of the args.

For example, in Gecko I receive HashMap<String, StringOrNumber> and have to convert it to first an allocated list of ids, and then a new hashmap of references to those ids and actual FluentValue values.

In result it's just one of those cases in Rust, where &str looks very pretty for examples, but becomes a headache in actual code.

My intention in this PR will be to add some macro and maybe Cow, to make it look more like:

fluent_args!(
  "name": "John",
  "emailCound": 5
);

[stasm]

OK, I see, thanks for the explanation.

In simple cases, the argument keys would be OK to stay as &'static str, but I understand how keeping &str makes it less convenient to use fluent-rs in higher-level bindings code.

My intention in this PR will be to add some macro

Macros hide complexity, rather than remove it. fluent-rs is a low-level package; it should be OK to expect the code to get slightly verbose. I'd stay await from macros in the public API. I vote for wontfixing this issue.

and maybe Cow

Do we need to be able to modify the argument names?

[zbraniecki]

Do we need to be able to modify the argument names?

Nope, but it would be nice to accept &str or String as a key.

[stasm]

Hmm, but even with Cow there's going to be a need to iterate over all keys of the hash map and Cow::from() or .into(), right? And besides, for higher-level code, it will probably need to iterate anyways, in order to wrap the values of arguments in FluentTypes. So perhaps it's OK to use &str here after all?

[zbraniecki]

Hmm, but even with Cow there's going to be a need to iterate over all keys of the hash map and Cow::from() or .into(), right?

Yes.

And besides, for higher-level code, it will probably need to iterate anyways, in order to wrap the values of arguments in FluentTypes.

That's another thing I'd like to provide a helper for.

So perhaps it's OK to use &str here after all?

Not really. What we should get to is a state where at least String and &'static str are supported. There's a PR for Rust to auto-cast that, but I need something sooner.

Quoting kornel from https://github.com/rust-lang/rust/issues/46966#issuecomment-392294359

[Using &str gives you] idiomatic examples of un-idiomatic Rust.

[zbraniecki]

@Manishearth:

For this item, I'd like to improve ergonomics of passing arguments to FluentBundle::format_pattern.

Currently, the args is of type HashMap<String, FluentValue>, which is often unnecessary since users define the keys as literals in their code. On the other hand, there are cases where user is fetching the argument keys from some other structure and has them as String. In that case, having to convert them to &str is not-trivial.

What I think I'd like to get to is:

a) What should be the type? I see HashMap<String, FluentValue>, HashMap<&str, FluentValue or some sort of HashMap<Cow<str>, FluentValue>.

This has been already discussed a bit in #94 and https://github.com/rust-lang/rust/issues/46966#issuecomment-392294359 but I'm not sure what the correct approach is for us.

(the keys only need to live long enough for the format_pattern to finish)

b) What kind of macro can we build to facilitate the args construction?

I'm thinking of something similar to vec![], but for maps. I see https://github.com/rust-lang/rfcs/issues/542 which indicates that it's not an easy problem to solve.

I thought of sth like:

let args = fluent_args![
  "name": "John",
  "emailCount": 5
];

but preferably it should also allow for:

let args = fluent_args![
  "name": FluentValue::String("John"),
  "emailCount": FluentValue::Number(5)
];

Do you have any thoughts on how to make it canonical for Rust integration and how to integrate it into (a)?

[Manishearth]

Not really, this is weird enough that there's no obvious ergonomic way to do it that pops up.

let args = fluent_args![
  "name": "John",
  "emailCount": 5
];

You can write a proc macro for the above, though.

[stasm]

I'd like to oppose the addition of the fluent_args! macro. fluent-rsfluent-bundle is a low-level library. From the "fast, flexible, ergonomic" triad, I think we should focus on fast and flexible. Just let people write the code they need, without hiding complexity in macros. Any addition to the public API extends its surface and increases the maintenance burden.

[zbraniecki]

The current PR places the macro in fluent-bundle crate, which I agree is low-level and moving toward stabilization. fluent-rs crate is more of a convenient catch-all crate that allows people to get the closest thing to user-friendly set of fluent-* crates and I planned to bundle fluent-fallback and fluent-resmgr into it.

Would you be ok to put the macro there? I understand the low-level aim for fluent-bundle and I agree with it.

[stasm]

Yes, I think that's a good compromise. Thanks for taking my input into account.