search

image-rs / image

Encoding and decoding images in Rust
Apache License 2.0
4.98k stars 618 forks source link

[API/Minor/Uncertain] Change acronym enum variants to CamelCase #974

Closed AregevDev closed 8 months ago

AregevDev commented 5 years ago

I noticed there that acronym enums has a SCREAM naming, which should be changed according to the naming conventions. This includes all the ImageFormat variants and some of the ColorType variants.

This is very minor and could be changed easily, however I wouldn't rush to do it as many projects use this crate and this change can break things...

First, I want to know if it is known and get a response.

Thanks!

birktj commented 5 years ago

See also https://github.com/image-rs/image/issues/741. https://stackoverflow.com/questions/15526107/acronyms-in-camelcase is also an interesting discussion.

birktj commented 5 years ago

@HeroicKatora what are your thoughts on this, I have thought about it before, but I have sort of mixed feelings. On one hand, more consistency with the api guidelines and the library itself would be nice, ~but the breaking changes are quite large and the question is if the (relatively small) improvements are worth it.~ Forget that, as I was writing it I realized that it is stupid, we have many more breaking changes to go before 1.0, and we should definitely do what we can to make the library as good as possible before then.

HeroicKatora commented 5 years ago

It's a good idea to change it for the color types for sure. For the formats I don't see such a great benefit but for consistency it should be done as well. This is relevant for #865 where the ColorType is getting updates anyways.

AregevDev commented 5 years ago

Should I change these and send a PR?

HeroicKatora commented 5 years ago

@AregevDev Feel free to send remaining changes to the branch next, e.g. the decoder still have this problem. It's been noted before that WebP is the more precise capitailization and we've also been in favor of this form in #1056. Remember that change for decoders, there we have WebpDecoder instead of WebPDecoder.

AregevDev commented 5 years ago

Ok, which changes do I need to submit then, in addition to the formats?

HeroicKatora commented 5 years ago

Most of the format specific wrappers, encoders. We could also remove the format prefix from these. The Rust api guidelines correct version is gif::Encoder, not dxt::DXTEncoder for example.

fintelia commented 5 years ago

I'm against all of our decoders to be called Decoder. If we're concerned about repeated typing, I think it would be way better just to place/re-export them at the crate root. Once we make the image-core split I think that this will make even more sense: each module would otherwise just be a couple of re-exports. (The same rule is often ignored for repeating the crate name in the types is exports so that wouldn't be an issue).

Fixing the capitalization would be good though.