phyrondev / openimageio

High Level Rust Wrapper for OpenImageIO
7 stars 0 forks source link

Method Naming Principles #1

Open virtualritz opened 4 months ago

virtualritz commented 4 months ago

Method Naming

For reference see:

No Prefix

Example candidates are all ImageBufAlgo method variants that take an additional dst as their 1st parameter.

For example:

ImageBuf::over(
    &mut self,
    b: &ImageBuf,
)
ImageBuf::rotate(
    &mut self,
    angle: f32,
)

Example usage:

/// Rotate `image_buf_a` 42 degrees.
image_buf_a.rotate(42.0);

/// Replace `image_buf_a` with `image_buf_a` *over* `image_buf_b`.
image_buf_a.over(&image_buf_b);

_with Postfix: Additional, Optional Parameters

A lot of methods take additional optional parameters on the C++ side. These get modeled with Option wrappers on the Rust side.

For convenience, there should be simple versions w/o the need to to specify these. The simple method has the short name. See above.

The one accepting additional parameters has the _with postfix.

Example of simple version:

ImageBuf::rotate(
    &mut self,
    angle: f32,
)

The full version:

ImageBuf::rotate_with(
    &mut self
    angle: f32,
    filter_name: Option<&str>,
    filter_width: Option<NonZeroF32>,
    recompute_roi: Option<bool>,
    roi: Option<Roi>,
    thread_count: Option<NonZeroU16>,
)

from_ Prefix: Contrustructors

These return a new object. Candidates are all the ImageBugAlgo method variants that return a new ImageBuf.

Example:

ImageBuf::from_over(
    &self,
    b: &ImageBuf,
)

Additinal parameter version (see above):

ImageBuf::from_over_with(
    &self,
    other: &ImageBuf,
    roi: Option<Roi>,
    thread_count: Option<NonZeroU16>,
)

try_ Prefix: Immediate Error Handling

All methods have a variant that starts with try_ and return (for now) anyhow::Result<()>.

ImageBuf::try_from_over_with(
    &self,
    other: &ImageBuf,
    roi: Option<Roi>,
    thread_count: Option<NonZeroU16>,
) -> Result<()>

For deferred error handling ImageBuf::has_error()/is_error() (convenience alias) and ImageBuf::error() can be used, just as on the C++ side.

virtualritz commented 4 months ago

To decide:

lgritz commented 4 months ago

I'm not familiar enough with Rust to know the conventions.

Can you not structure the IBA functions as freestanding functions like we do in C++? Must they be object methods of ImageBuf? How would that not be inconvenient by having to change the definition of ImageBuf to accommodate every new IBA function? Or am I misunderstanding a bit of Rust culture?

virtualritz commented 4 months ago

In Rust "the defition" is just the struct.

Methods are in an impl block and you can have as many such blocks for a single struct as you like. And they can also be in different source files inside the same crate.

And they get separated in the autogenerated documentation. Rust gets always compiled from source, there is no ABI that can break.

Considering the above, do you see any other other reasons why I should copy the C++ approach instead of going with my initial suggestion?

lgritz commented 4 months ago

Can users easily write these additional methods, too? Or do they all need to be in the crate we deliver?

The main reason for copying the C++ approach is just the symmetry and ease of documentation. But you need to weigh that against the desire to express things with a Rust-native accent. If it's an arbitrary choice, I would say try to stick close to C++. But if fluent Rust users would look at a freestanding IBA function and say "yuck, those were clearly written by somebody who only has used Rust for 3 days, nobody skilled would do that", then definitely diverge as needed.

virtualritz commented 4 months ago

Can users easily write these additional methods, too? Or do they all need to be in the crate we deliver?

Yes, via user-defined traits. E.g. another crate can contain this:

use openimageio as oiio;

trait ImageBufFoo {
    fn foo(&mut self, other: &ImageBuf);
}

impl<'a> ImageBufFoo for oiio::ImageBuf<'a> {
    fn foo(&mut self, other: &ImageBuf) {
        self.over(other, None, None);
    }
}

And after importing this 3rd party crate a user of both crates can call:

my_image_buf_a.foo(&my_image_buf_b)`.

The main reason for copying the C++ approach is just the symmetry and ease of documentation.

I honestly think Rust users should not need to use openimageio.readthedocs.com.

I.e. for starters I will copy all releveant docs from there (edited where needed) into the crate so that the autogenerated docs.rs ones contain the same amount of reference documentation and no guessing is needed when stuff diverts for canonical reasons.

I will also add a special section to each type or method that diverges from the C++ one in any way (including a brief reason).

For anything that has tutorial/guide character and is not specific to a type or method, there could be an mdbook, eventually. But I don't see this at all in the readthdocs ones, unless I miss something.

Finally, for renamed types there will be type aliases to make C++ people feel at home/make the native speaker crowd that don't use an autocompleting IDE happy.

An good example is probably RegionOfInterest which can also be referenced as Roi. I.e. while a C++ developer could do:

use openimageio::RegionOfInterest as Roi;

or

use openimageio::RegionOfInterest;

type Roi = RegionOfInterest;

They won't have to. The latter type alias will already be defined in the openimageio crate.

But if fluent Rust users would look at a freestanding IBA function and say "yuck, those were clearly written by somebody who only has used Rust for 3 days, nobody skilled would do that", then definitely diverge as needed.

There is really no good reason to do this on the Rust side, IMHO.

For writing literal code, the syntax using self is usually more easy on the eyes. I.e. a.over(b) vs ImageBuf::over(a, b). Also considering the above, e.g.:

/// Replace `a` with `a` over `b`.
a.over(b);

/// Put `a` *over* `b` into `c`.
let c = ImageBuf::from_over(a, b);
virtualritz commented 4 months ago

All that said, you can of course still call the self version explicitly anyway.

I.e. you can still call ImageBuf::over(a, b) and this is the same as a.over(b). The latter is just syntactical sugar the use of self affords. So if you're coming from C++ and you want the code to look similar, you can always do:

use openimageio::*;

type ImageBufAlgo = ImageBuf;

...
/// Replace `a` with `a` *over* `b`.
ImageBufAlgo::over(a, b);

/// Put `a` *over* `b` into `c`.
let c = ImageBufAlgo::from_over(a, b);
lgritz commented 4 months ago

I.e. you can still call ImageBuf::over(a, b) and this is the same as a.over(b). The latter is just syntactical sugar the use of self affords.

Ah, ok, all fine then.