Feature: Extend CLI #14
Conversation
dcvz
force-pushed
the
feature/extended-cli
branch
from
3307329
to
01cd7e6
Compare
| // Convert the image | ||
| let mut bin: Vec<u8> = Vec::new(); | ||
| if let BinaryFormat::Palette = args.format { | ||
| pigment64::create_palette_from_png(&mut input_reader, &mut bin)?; |
There was a problem hiding this comment.
I see some places say crate:: and others say pigment64::. Is it needed to be different?
There was a problem hiding this comment.
Yeah, so that has to do with when something comes from the library and what comes from within the cli package. Things prefixed with pigment64 are being imported from the lib - the cli is a user of the lib. Things with crate are within the cli package and are not part of the lib.
| BufWriter::new(output_file).write_all(&bin)?; | ||
| } | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
Any reason for returning an empty Ok(()) instead of just a void return? I don't see any Error anywhere
There was a problem hiding this comment.
It can be hard see unless you’re familiar with it, but some things that /could/ error are being handled with ? at the end, which would bubble the error out. If you make it to the last Ok(()) then nothing before failed.
There was a problem hiding this comment.
Oh, that's what ? mean. Gotcha, thanks.
src/cli/defines.rs
Outdated
| BinaryFormat::Ia16 => ImageType::Ia16, | ||
| BinaryFormat::Rgba16 => ImageType::Rgba16, | ||
| BinaryFormat::Rgba32 => ImageType::Rgba32, | ||
| _ => panic!("cannot convert palette to native format"), |
There was a problem hiding this comment.
Wouldn't be better to use ::Palette explicitly here?
There was a problem hiding this comment.
Maybe? I know we explicitly will only handle these no matter what other items we may add to facilitate pigment use. So that’s why I went for this option. I’m fine either way.
src/cli/defines.rs
Outdated
| BinaryFormat::Ia16 => ImageSize::Bits16, | ||
| BinaryFormat::Rgba16 => ImageSize::Bits16, | ||
| BinaryFormat::Rgba32 => ImageSize::Bits32, | ||
| _ => panic!("cannot convert palette to native format"), |
| @@ -94,6 +94,17 @@ impl PNGImage { | |||
| } | |||
| } | |||
|
|
|||
| /// Writes the image as a PNG to the given writer. | |||
| /// This is useful for when you need to flip an existing image. | |||
| pub fn as_png<W: Write>(&self, writer: &mut W) -> Result<()> { | |||
There was a problem hiding this comment.
It may be more clear to have this function named as write_as_png?
There was a problem hiding this comment.
We’re using this naming semantic everywhere else so I kept it. We’re writing data to some medium for sure. I don’t feel strongly about it. Do you find this name confusing?
There was a problem hiding this comment.
Oh, if this naming scheme is already being used then it is fine.
I think it may be a bit confusing to read something like image.as_png(), but that discussion is out of scope for this PR
This PR extends the CLI tool to handle both converting from binary > png and vice versa. As part of that it refactors the CLI code to handle subcommands.