Passing optional arguments to wrapped `ImageBufAlgo` methods.

[virtualritz]

There are three approaches I considered so far:

Init-Struct Pattern

I am leaning towards this one as it is easy on the eyes when reading the code and not too verbose.

Example:

/// Defined inside `openimageio` crate:
pub struct RotateOptions<'a> {
    pixel_filter: &'a Filter2D,
    recompute_region_of_interest: bool,
    region_of_interest: RegionOfInterest,
    thread_count: u16,
}

/// Call site in user code:
let pixel_filter = Filter2D::BlackmannHarris(4.0, 4.0),

my_image_buf.rotate_with(
    42.0,
    Some(RotateOptions {
        pixel_filter: &pixel_filter,
        recompute_region_of_interest: true,
        Default::default(),
    }
);

Optional Arguments

See also #1. There would be two versions of each method, one that takes only required arguments and one with a _with postfix taking optional arguments as Options.

The problem is that Rust doesn't have named arguments. The code becomes harder to understand w/o comments.

E.g., what does Some(true) mean below?

/// Call site in user code:
let pixel_filter = Filter2D::BlackmannHarris(4.0, 4.0),

my_image_buf.rotate_with(
    42.0,
    Some(&pixel_filter),
    Some(true),
    None,
    None,
);

Builder Pattern

Lots of boilerplate needed, not better than init-struct, just different.

/// Call site in user code:
let pixel_filter = Filter2D::BlackmannHarris(4.0, 4.0),

my_image_buf.rotate_with(
    42.0,
    Some(RotateBuilder::new()
        .pixel_filter(&pixel_filter)
        .recompute_region_of_interest(true),
        .build()
    )
);

[vvzen]

As somebody who had to recently refactor a big lib+CLI combo to add a few more new arguments to a function call (which was called 96 times!), I feel either using an Init-Struct or a Builder-Pattern would be a good approach. The main difference between the two is that the builder pattern can be a little bit less heavy since on the client side, at the cost of the library being more boilerplate-y, since you can "hide" the default parameters within the impl of the Builder, while with the Init struct the caller has to explicitly pass Default::default() : not a big deal, but worth keeping in mind.

Also: can I ask you the rotation itself is not part of the arguments ? I'd expect that passing 0 as rotation or not passing any rotation would be equivalent, so it could also be modeled as optional too ? :thinking:

You could also avoid having to wrap everything in Some() by hiding all of the optionals in the library code . So the call side could write just:

my_image_buf.rotate(
    RotateOptions::builder()
        .degrees(42.0)
        .pixel_filter(&pixel_filter)
        .recompute_region_of_interest(true)
        .build()
);

which means that just rotating using defaults for everything expect the rotation would look like this:

my_image_buf.rotate(
    RotateOptions::builder()
        .degrees(42.0)
        .build()
);

What do you think ?

[virtualritz]

I very much prefer the init-struct pattern. I think Default::default() is also a nice way to say (to someone else reading the code): Be aware that the rest of this is using defaults! (literate programming)

This is not so obvious when using a builder.

[...] while with the Init struct the caller has to explicitly pass Default::default() : not a big deal, but worth keeping in mind.

Likewise, you still need to write ::builder()/::new() at the beginning and build() at the end here. But unlike Default::default() the latter doesn't convey any info in its name about what is taking place, behind the scenes. You could of course call the last function build_using_defaults_for_the_rest() or sth along those lines. But why not just save some typing to the same effect and with a ton less boilerplate?

Even if you use sth. like derive_builder that boilerplate code still gets generated and has to be compiled. Build times are an issue on Rust. That is an acceptable tradeoff if there is a gain for a user of the crate. But I don't see this here.

I think structs that can be initalized to member configurations that are simply 'wrong' (whatever that means) should indeed be opaque (i.e. all resp. members be private) and a builder be used.

For example RegionOfInterest (aka Roi aka ROI in C++) may be such a struct but there is a workaround here where a special case is covered turning this into a sum-type on the Rust side instead (that's what I settled on, for now). See here for a discussion on the OIIO Slack.

But this is not the case here (and for most of optional parameters in OIIO, especially in ImageBufAlgo).

Also: can I ask you the rotation itself is not part of the arguments?

The angle itself is not part of the options because it is not optional. The call makes no sense when angle is 0. I would think the C++ side probably has a check for this that makes the op fall through in this case (resp. return a memcpy'd version of the input in the C++ version of rotate() that takes a dst ImageBuf, i.e. from_rotate…() on the Rust side).

The rules are:

1. Everything that is does not have a default on the C++ side is a function parameter on the Rust side The API actually matches 1:1.
2. Everything that has a default (is optional) on the C++ side becomes part of the resp. …Options struct on the Rust side. The Rust API diverts from the C++ one after the last non-default function parameter -- but only in the …_with variants -- see below.

So far I have four variants of most ImageBufAlgo functions:

impl ImageBuffer {
    pub fn rotate(&mut self, angle: f32) -> Result<&mut self> { … }
    pub fn rotate_with(&mut self, angle: f32, rotate_options: &RotateOptions) -> Result<&mut self> { … }
    pub fn from_rotate(angle: f32) -> Result<ImageBuffer> { … }
    pub fn from_rotate_with(&mut self, angle: f32, rotate_options: &RotateOptions) -> Result<ImageBuffer> { … }
}

Note also that the 1st two methods return &mut self so they allow chaining. :grin:

[virtualritz]

The main difference between the two is that the builder pattern can be a little bit less heavy since on the client side, at the cost of the library being more boilerplate-y, since you can "hide" the default parameters within the impl of the Builder, [...]

I don't quite get this. Builder version below (w/o spaces) is 95 characters:

    RotateOptions::builder()
        .pixel_filter(&pixel_filter)
        .recompute_region_of_interest(true)
        .build()

vs init-struct pattern, 100 characters:

    RotateOptions {
        pixel_filter: &pixel_filter,
        recompute_region_of_interest: true,
        ...Default::default()
    }

I wouldn't call a five character difference 'heavy' but if you do, that is yet another reason for not making non-defaults parameters part of the builder: they don't require the latter then -- even less typing. :wink:

my_image_buf.rotate(42.0)?;

[vvzen]

I very much prefer the init-struct pattern. I think Default::default() is also a nice way to say (to someone else reading the code): Be aware that the rest of this is using defaults! (literate programming)

That's true! However, in my experience with languages with named args (like Python) it's never really been a problem/source of confusion the fact that you don't see an explicit call to something akin to Default::default(). When you can call a fn in Python the caller just passes the kwargs you care about, you don't need to spell out anything else (like one has to do with Default::default() in Rust).

In any case, YMMV - what's verbose for some it's self-documenting code for others! I don't think there's anything wrong with the Init struct pattern! And I've personally used it plenty of times. One thing to consider is API breaking changes - I don't have a lot of experience there and I have a vague feeling that the builder makes it easier to avoid breaking changes but I might be wrong, I need to read https://github.com/rust-lang/rfcs/blob/master/text/1105-api-evolution.md#structs in depth.

The angle itself is not part of the options because it is not optional. The call makes no sense when angle is 0.

That's^ the kinda of stuff that I can see one would like to encode in the type system instead of leaving it to the diligence of the caller, but maybe it's overkill here.

Re your second post, you are mentioning just RotateOptions without wrapping it in a Some(RotateOptions { .. }) , which is what your first post here was doing - this is what my comment was pointing to. The client doesn't need to wrap anything in Option explicitly because you can do it in the library implementation of the RotateOptionsBuilder.

So you would have:

my_image_buf.rotate(
    RotateOptions::builder()
        .degrees(42.0)
        .pixel_filter(&pixel_filter)
        .recompute_region_of_interest(true)
        .build()
);

vs your previous example, where the extra options where wrapped in an Option :

my_image_buf.rotate(
    42.0,
    Some(RotateBuilder::new()
        .pixel_filter(&pixel_filter)
        .recompute_region_of_interest(true),
        .build()
    )
);

In any case, if you end up having 2 methods (one w/ no options and one w/ extra options), I think this is a non-issue (not that it was a huge issue anyway).

You can just have:

my_image_buf.rotate(42.0)

and

my_image_buf.rotate_with_options(
    42.0,
    RotateOptions::builder()
        .pixel_filter(&pixel_filter)
        .recompute_region_of_interest(true),
        .build()
    )
);

or, via struct init:

my_image_buf.rotate_with_options(
    42.0,
    RotateOptions {
        .pixel_filter: &pixel_filter,
        .recompute_region_of_interest: true,
        ..Default::default()
    }
);

Also note that you're referring to RotateBuilder::new() but that's not the idiomatic way to call it on the client side, a builder should be discoverable from the struct itself, which is why I'm using RotateOptions::builder() instead.

The builder is also used extensively in the std, e.g.: https://doc.rust-lang.org/std/process/struct.Command.html (as mentioned by https://rust-unofficial.github.io/patterns/patterns/creational/builder.html) so that's another thing to keep in mind.

Also, didn't want to start a bike-shed on this, I was just happy to see some movement here in OIIO/Rust-land! :smile:

[virtualritz]

Yeah, when I opened the ticket I didn't have the split into _with-variants yet.

If you look at the four signatures I gave in the original reply to you, you'll notice they already do not have the Option-wrap any more (the directrly take &RotateOptions).

Because of Rust's zero-cost abstractions, using Some(&RotateOptions::default()) isn't different, resulting assembly wise, from just using &RotateOptions::default() directly.

However, The former can be abbreviated with None, which is less expressive (None of what?) but shorter. That was the original reason for having the Option-wrap. But from a literate programming pov, that's not so nice.

But once you have _with variants the user just calls that variant if they have to pass anything (no Option-wrapping needed) or the non-postfixed version otherwise.

I find this cleaner (not least on the eyes) than having a single version of the resp. function that then gets called with None all of the time.

In any case, if you end up having 2 methods (one w/ no options and one w/ extra options), I think this is a non-issue (not that it was a huge issue anyway).

You still need four variants. Two are constructors that create & return a new ImageBuffer.

The C++ API btw. also usually has four overloaded variants, for similar reasons.

[virtualritz]

The builder is also used extensively in the std, e.g.: https://doc.rust-lang.org/std/process/struct.Command.html (as mentioned by https://rust-unofficial.github.io/patterns/patterns/creational/builder.html) so that's another thing to keep in mind.

Yes, builders are much older than init-struct. Also init-struct has the issue of missing struct-exhaustiveness possibly breaking existing code when new struct members are added on the API-side and the user has specified all (previous) members and thus omitted ..Default::default().

I think of this as an advantage though as you automatically get notifed of the API change by the compiler in that case. Solutions: look up the new stuff you missed now and add it or simply add the missing ..Default::default(). The real issue is that there is no compiler support for the init-struct idiom so there is no error message that hand-holds you. Curiously this is something one starts to care about after using rustc with its awesome error messages (mostly) for a while. :grin:

The quoted blog post suggest adding a #[doc(hidden)] empty pub member to the struct which is named __non_exhaustive and then coerces the user to always use ..Default::default()

Both builder- and init-struct patterns are really workarounds for the lack of named arguments in Rust. :disappointed:

[virtualritz]

Named arguments are btw. only half of what is needed to resolve this issue on the compiler-side. You also need defaults for arguments which is another can of worms.

Because for something that has a bunch of (named) arguments and defaults not being available in the compiler, all arguments have to be written out still, even if they're all default (None).

See the 2nd example (Optional Arguments) in the original issue descr. That's a lof of code doing nothing that still has to be written (and distracts a reader) for zero gain.

TLDR; if named arguments were added to rustc tomorrow (w/o also adding support for argument defaults), this issue would remain to really only be solveable with a builder- or init-struct pattern. :disappointed:

[vvzen]

I think of this as an advantage though as you automatically get notifed of the API change by the compiler in that case. Solutions: look up the new stuff you missed now and add it or simply add the missing ..Default::default().

Yeah that's one way of looking at it - however if everybody followed that approach in the crates ecosystem it would probably be death by a thousand papercuts :smile: Imagine having to release a bugfix because your CI broke just because a dependency (maybe a transitive one!) updated a struct and you (or a dep you use) were not using Default::default(). The fact that the client needs to know about using it makes it a bit of a leakier abstraction, imho.

Named arguments are btw. only half of what is needed to resolve this issue on the compiler-side. You also need defaults for arguments which is another can of worms.

Yep - I agree. To be honest, in the last ~3 years that I've been using Rust, I didn't miss named args that much. And when I really wanted to provide self-documenting but rust-y ergonomics, I wrapped stuff into macros that looked like they had named args, e.g.:

let something = my_macro!(thing = 1, thong = "hello");

Of course with macros comes longer compile times and harder error messages, but life is about tradeoffs. In my case I haven't really found compile times to be unbearably long.

TLDR; if named arguments were added to rustc tomorrow (w/o also adding support for argument defaults), this issue would remain to really only be solveable with a builder- or init-struct pattern. 😞

Yep. BTW there's not even an RFC on named args, I doubt they will ever make into the language before a 2.0: https://github.com/rust-lang/rfcs/issues/323.

But once you have _with variants the user just calls that variant if they have to pass anything (no Option-wrapping needed) or the non-postfixed version otherwise.

I find this cleaner (not least on the eyes) than having a single version of the resp. function that then gets called with None all of the time.

Yeah I agree - I think that's much better for LSPs/autocompletion and also doc-comments/PRs readability!

PS: There's also https://docs.rs/bon/latest/bon/ to help create builders ~for functions~ with less work