Throw exceptions on errors instead of writing to stdout about nimpng

[liquidev]

An error enum is another option. I that case, I think the API would become:

var png: PngResult
let errCode = png.load32("file.png")
if errCode == peNotAPng:
  echo "file is not a png"

[mratsim]

I would prefer if it uses errors. PNGs reading is the type of library that are valuable to use as DLLs and try-catching across DLLs is very strange. An error enum would be easier to embed and use and also it would be easier to prevent API regressions for example if suddenly the exceptions thrown changed, a change in error returned instead would be visible in the type changes.

[jangko]

If you take a closer look, error conditions can be detected from the return value, but currently they are not documented.
Error condition will return either nil, zero len string, or false.
They were designed with simplicity in mind rather than uniformity or sophisticated design.
Basic functionality came first, cosmetics and additional features come next.

I would prefer to keep the current API as is. Additional features will be implemented using a new set of API. This way will prevent breaking changes.

So far me and some other people I know using this library for GUI app and video games without any significant problem. Error condition can be handled very well. this library is well tested and used in various fool proof system.

[liquidev]

It does work well for apps, it's just that better error handling with nice messages could potentially be implemented.

Another option like isValidPng or isError or something that doesn't break existing APIs would be good, but you'll have to agree with me that outputting error messages to stdout is a bad idea, especially for CLI apps. stderr should be used for errors and messages instead, but outputting to the standard libc output files is widely considered bad practice anyways.

[jangko]

Initially, I thought using string as pixel value/bytes container is a good Idea. But it is not true.
Bad design already accumulate for the sake of simplicity.

I'm not against your opinion. I agree we should improve this library. The error message not the only issue. Patching existing API will only increase more bad design decision and convolution.

We need a new set of API that is well designed in terms of ergonomicity, efficiency, consistency, including error handling.

PR are welcome.

[liquidev]

Initially, I thought using string as pixel value/bytes container is a good Idea. But it is not true.
Bad design already accumulate for the sake of simplicity.

Yeah, I already encountered problems with using strings (namely value out of range errors when trying to use 255 as a value).

We need a new set of API that is well designed in terms of ergonomicity, efficiency, consistency, including error handling.

PR are welcome.

I'd rather discuss this redesign with other people first. What are your ideas for a better API?

[jangko]

We can replace string with openArray[byte] for input, the internal need some refactoring.
The output pixels data can use seq[byte] or make it generic for backward compatibility.

For the error code we can use PNGEncoder and PNGDecoder with additional field.
The IO API(save and load) will have new API with new name.

etc. :)

[liquidev]

We can replace string with openArray[byte] for input, the internal need some refactoring.
The output pixels data can use seq[byte] or make it generic for backward compatibility.

Actually, I think using a generic seq[T] with a constrained type (to uint8 | uint16 | float) would be a better option, since we can support various PNG pixel formats without any bigger issues.

I like the idea of an error code being stored in PngEncoder and PngDecoder, how would you handle this in (load|decode|save|encode)Png(|24|32) though?
My idea was to use a var PngResult parameter for load and decode and returning the error code from the functions, save and encode would not need PngResult in this case. By the way, I think that PngResult isn't the best name, something like PngImage would be better, because then we could reuse the same object for both loading and saving.

[jangko]

Actually, I think using a generic seq[T] with a constrained type (to uint8 | uint16 | float) would be a better option

openArray[T] could accept seq[T], but not the other way around.

PngResult isn't the best name, something like PngImage would be better, because then we could reuse the same object for both loading and saving.

yeah, they were implemented in hurry.

how would you handle this in (load|decode|save|encode)Png(|24|32) though?

If we are going to use PngImage, it will be our main data structure to communicate with the rest of the world.

[liquidev]

I'll start by trying out some ideas. Does this project have some specific style guidelines? I usually just go with NEP-1 and the stdlib naming conventions, but from what I can tell nimPNG's code style is a bit different (eg. PNGResult instead of PngResult)

[jangko]

NEP-1 will be ok. nimPNG born when I was still learning Nim. Many things at that time still unclear for me. The only thing clear at that time is the PNG spec.

[jangko]

btw we have ranges, endians2 and result in https://github.com/status-im/nim-stew.
probably we can use them to improve the design of nimPNG.