Closed zbraniecki closed 4 years ago
Here are my general thoughts on the design of such APIs:
Positional parameters are fine, as long as there isn't 10 or 15 of them.
Position bool parameters might suggest that the function should really be two functions. In particular when there's only one bool parameter passed as a flag. I think it can be argued that this is the case here.
Your proposed option (2) looks good to me. I might suggest calling the other method add_resource_overriding
, but it's a tiny personal preference.
I don't think we need to change fluent.js
in the same way, by the way. The bag-of-options pattern combined with default parameter values works well enough there.
One way the Rust community has tried to work around the lack of default parameters, at least in constructors, is through the builder pattern. I'm not sure if I like it, mostly because I'm not a fan of the so-called "fluent interface" API design (name not related to our Fluent), but I see its value. I don't know if it applies well to methods, however. Perhaps something like the following could work:
bundle.with_resource(&res1).add().with_resource(&res2).add_overriding();
Or even:
bundle.with_resource(&res1).with_resource(&res2).add_overriding();
That's a larger change, however, and I don't think we should do it just for one bool parameter.
The Default
trait looks interesting, but again, I don't think it's worth it for just one parameter.
Which prompts the follow-up question: how do we pass arguments to the FluentBundle
constructor? I mean the use_isolating
option and the transform
function. It looks like we don't? Instead, they're public and can be set after instantiation, right? I think that's perfectly fine, by the way, and I'd keep it that way.
There are some differences in result of how Rust impl treats the constructor options - for example in Rust you can turn isolating on/off without having to recreate an instance. That's fixable and we can in the future consider adding setIsolating
to FluentBundle
in JS. Just mentioning.
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:
2) Separate API:
3) Options bag with defaults:
allowing for:
or maybe even
Option<FluentBundleAddResourceOptions>
to allow foradd_resource(&res, None)
or maybe even:
========
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.