kornelski / rust-rgb

struct RGB for sharing pixels between crates
https://lib.rs/rgb
MIT License
97 stars 19 forks source link

Tweaks to the new commits on `v0.9` #118

Closed ripytide closed 1 month ago

ripytide commented 1 month ago

Thanks @kornelski for the all the commits you've added to the 0.9 (now 0.9-old I think), branch, nearly all of them are amazing changes. I've gone through them all (they're very well organized too!) and compiled some tweaks, mainly doc-related.

The one non-doc change which I think may not be a good idea is moving the TryFrom error types into a top-level module error. When writing out an import statement I no longer get auto-complete after typing use rgb:: for those types. So I'd need to start memorizing the name of the module they are in, error, which makes typing longer for no benefit I can see at the moment since no other types/traits in the library begin with "T" at the moment.

kornelski commented 1 month ago

I'm concerned about the noise on the docs.rs page for the crate. I assume the error types will be very rarely used.

kornelski commented 1 month ago

I don't think there's ambiguity in RGBW::new. The RGB part is in the "correct" order. With other types the problem is that new differs from the popular RGBA convention. If the pixel was WRGB, then it'd need a clarification.

ripytide commented 1 month ago

I think I would prefer a more complex doc.rs home page to nicer imports since I can easily skim over types when reading the docs. This is very marginal though, I think I just like the consistency of having everything exported from the top-level. Not a blocker for me.

Rgba::new() could change to Rgbw::new(), perhaps unknowingly via a type alias:

type Pixel = Rgba;
//type Pixel = Rgbw;

let pixel = Pixel::new(255, 0, 0, 255);

Since they both have the same number of arguments this would still compile when switching pixel types which could cause hard to catch bugs, which is prevented by changing the new() to new_rgbw().

kornelski commented 1 month ago

I've pushed the other commits.

I don't think even the new substituted via alias is a problem. People who need RGBW are probably already fudging it via RGBA types in APIs that don't support RGBW explicitly (and most don't), so A == W is not even a bug.

ripytide commented 1 month ago

From #121 I think another interesting use-case for having everything exported from the top-level is that you can use a rgb::* glob import to get every type/trait in the library into scope which wouldn't work with the errors module. I don't think this is a super-common use-case but when relevant it could be a useful property. Although a prelude module may also break this property.

Perhaps that is also the reasoning behind wgpu doing the same?

kornelski commented 1 month ago

The errors are only for iterators that don't have enough elements. I don't expect them to be something that users explicitly and carefully handle, because getting the number of channels wrong is usually just a bug. I assume most users will know they have an iterator with the right number of elements, and just .unwrap() it, similarly to how u32::from_le_bytes(slice.try_into().unwrap()) is used with an always optimized-out unwrap.

In case someone actually takes advantage of the fallibility for something like a parser, they may prefer to use .map_err to provide their own error anyway (to have an enum of their own error type, add the exact source code location, etc.). .unwrap_or_default() or iter.chain(repeat(0)) may be another option for dealing with potentially truncated/dodgy data. So using the error type explicitly by name is going to be a rare case of an API that itself is rarely used, since for such small fixed-size copy types, slice/vec casting and conversion from arrays are probably much more common.

ripytide commented 1 month ago

I agree that the types will be rarely used, but don't agree that putting rarely used types into modules is an improvement, as creating a module adds another item to the crate which can be imported and needs documenting, increasing complexity.