Fix BGR::new() argument ordering for v0.9

[ripytide]

#32 discovered the unintuitive argument ordering for various new() functions such as BGR::new().

These functions were then deprecated in v0.8.16, since for v0.9 we are allowed breaking changes I would like to fix this issue by re-ordering the function arguments to the intuitive ordering (b, g, r), and then un-deprecate them.

[lo48576]

new() could still be confusing even with bgr... people will stop to wonder “oh wait, is this (r, g, b) or (b, g, r)?” and have to read the documentation to make sure. So I personally prefer them only to have from_{rgb,bgr}-like explicitly named constructors.

Certainly BGR::new(b, g, r) is more justifiable since From<[T; 3]> for BGR<T> for [b, g, r] and From<(T, T, T)> for BGR<T> for (b, g, r) already exists (and I'm not strongly feeling they should be removed). They are less confusing since they are consistent with Into<[T; 3]> for BGR<T> and Into<(T, T, T)> for BGR<T>, i.e. combined Into & From makes round-trip conversion.

In contrast, BGR::new has no such "obvious counterpart"... the type can have both of to_rgb() and to_bgr() (while currently neither of them exist). ComponentSlice<T> can be it, but then the counter-counterpart might better be named from_components, ... while new is the conventional name for "from components"-like constructor???

The use of new(b, g, r) can be justified in many ways. I think, however, the real problem is not the lack of justifiability, but that the name provides non-zero possibility of receiving (r, g, b). The name new does not contradict the idea of “let's create BGR value from “a color”, which is conventionally represented by (r, g, b)!”

[ripytide]

new() could still be confusing even with bgr... people will stop to wonder “oh wait, is this (r, g, b) or (b, g, r)?” and have to read the documentation to make sure. So I personally prefer them only to have from_{rgb,bgr}-like explicitly named constructors.

I feel like LSP-enabled IDE's should prevent this since they often show you the function parameter names as you are writing a call to the function.

In contrast, BGR::new has no such "obvious counterpart"... the type can have both of to_rgb() and to_bgr() (while currently neither of them exist). ComponentSlice can be it, but then the counter-counterpart might better be named from_components, ... while new is the conventional name for "from components"-like constructor???

I'm not sure I understood this bit. Do mean in contrast to RGB::new()? Why would BGR have a to_bgr()?

The use of new(b, g, r) can be justified in many ways. I think, however, the real problem is not the lack of justifiability, but that the name provides non-zero possibility of receiving (r, g, b). The name new does not contradict the idea of “let's create BGR value from “a color”, which is conventionally represented by (r, g, b)!”

It's a good point, however, in my view a library has to take a stand at some point that users have to have a certain level of knowledge about the parameter ordering of the functions they are calling.

The alternative side with only offering from_rgb() and from_bgr() is that as a user now might not understand whether to call from_rgb(Rgb{r, g, b}) and from_bgr(Bgr{b, g, r}) or from_rgb(r, g, b) and from_bgr(b, g, r) so I'd still need to look at the docs at which point new() would work too.

There are also those users who would instinctively reach for a new() method upon wanting the construction of a new item like BGR and then upon finding the method look at the arguments to see what they need to give in order to create that object. Not having a new() method would cause friction to those users who would then need to open up a list of methods of the type in order to find a constructor method.

[kornelski]

new_bgr(b, g, r) and new_rgb(r, g, b) could work

[lo48576]

I feel like LSP-enabled IDE's should prevent this since they often show you the function parameter names as you are writing a call to the function.

Generally, the environment will be limited on readers side than writers side, for example some people will read them through git show, less, web browsers, and something like that. Writers will remember what they are using, but readers (including possible future writers) will read this without being sure. Of course they will continue reading if they entirely trust the developer, but it means that they cannot verify that part on the fly (without opening the document).

Why would BGR have a to_bgr()?

It won't be unnatural if BGR::to_rgb(self) -> (T, T, T) is provided... it would be convenient if users can write external_crate::process_rgb_tuple(RGB::from(my_bgr_color).into()) instead of external_crate::process_rgb_tuple(my_bgr_color.to_rgb()). Then providing BGR::to_bgr() as the shorter and consistent name would also be natural, because the essentially identical function <BGR as Into<(T, T, T)>::into() is already provided.

a library has to take a stand at some point that users have to have a certain level of knowledge about the parameter ordering of the functions they are calling.

I agree that the developer should be sure what they are writing, but non-author of the project will have less knowledge at first as described above. It's not critical, but it would be good to make the code reading easier even for the first-time reader... as far as it does not lower the usability of this crate.

The alternative side with only offering from_rgb() and from_bgr() is that as a user now might not understand whether to call from_rgb(Rgb{r, g, b}) and from_bgr(Bgr{b, g, r}) or from_rgb(r, g, b) and from_bgr(b, g, r) so I'd still need to look at the docs at which point new() would work too.

Totally true. I overlooked that since I usually use from() for such type-conversion-like construction.
Now I also think from_bgr() is less obvious...

Not having a new() method would cause friction to those users who would then need to open up a list of methods of the type in order to find a constructor method.

Sounds reasonable. I just remembered that I got a build error on Foo::new(), opened the document, and discovered that it only had Foo::default() for creation... That was actually a bad experience.

[lo48576]

I felt the name of BGR::new_bgr() was redundant as of writing the first comment. But now, considering the ambiguity of from_bgr(), it sounds great. It will be successfully suggested by IDE/LSP if the user types BGR::new, so developers will find this promptly without opening the browser.
(By the way, BGR::new_rgb() sounds like a contradiction? 😅)

[lo48576]

Another option is with_bgr and with_rgb. It solves the ambiguity, but users trying to write BGR::new won't find this without opening the document.

[ripytide]

As a reader reading BGR::new_bgr(0, 10, 100) there is still a non-zero chance I could interpret this as creating BGR {b: 100, g: 10, r: 0), especially if I didn't know of the existence of the BGR::new_rgb() method since it now just looks like a regular new() method. At which point we are back to square one in terms of readability.

I guess it might be a slightly reduced probability but as a non-trusting reader of that code I'd still want to look up the docs.

[kornelski]

I don't see a way of avoiding such ambiguity for a multi-arg function call, without Rust adopting something like Swift's syntax, or maybe BGR::b_is_first_then_g_and_r_is_the_last(0, 10, 100).

[ripytide]

There is also the option of removing the function completely as that would completely remove ambiguity if users are forced to use BGR { b: 0, g: 10, r: 100 }, although that would either mean also removing the RGB::new() method for consistency or leaving it and pixel types having inconsistent inherent methods.

On the other hand, this ambiguity is still technically an issue for impl From<(T, T, T)> for BGR<T> I suppose.

[lo48576]

or leaving it and pixel types having inconsistent inherent methods.

I feel the asymmetry here can be acceptable, because it is sourced from widely-shared "r, g, b" convention. In that sense BGR is second-class citizen (actually this crate is rgb and BGR is in alt module!). Having new would be a privilege of types with "conventional" (in other words, "obvious") representation.

About From<(T, T, T)> for BGR<T>, I'm not sure it should be retained or removed.
I'll personally prefer more explicit methods like BGR::to_bgr_tuple() -> (T, T, T) if available, but not all people will be so, and I have no idea of shorter yet clear names. (yet another bikeshed...)