search

pedrocr / rawloader

rust library to extract the raw data and some metadata from digital camera images
GNU Lesser General Public License v2.1
304 stars 53 forks source link

CFA Result instead of panic #14

Closed johannesvollmer closed 6 years ago

johannesvollmer commented 6 years ago

Made CFA::new_from_tag return a Result<CFA, String> instead of panicking when encountering an invalid tiff tag cfa color.

Also simplified CFA::clone by using a derive instead of manual copying.

pedrocr commented 6 years ago

These changes in themselves look fine but trying to decode something without a CFA pattern isn't going to be a good experience. What file are you trying to process?

johannesvollmer commented 6 years ago

Oh, I have not yet encountered a file with invalid tiff data. I understand that this may seem like an unnecessary change. Nevertheless, I don't want my application (which uses rawloader) to just exit when the user tries to load an invalid file.

I have learned that panics should only be used where the developer does something wrong, like errors in code. Invalid usage of the released product by Clients should probably not result in exiting the program and losing all unsaved changes, but instead the user should be displayed a nice error message. If I remember correctly, Results are a more idiomatic for nice error messages than panics, don't you agree?

Of course, there will be more changes necessary to achieve that, but I thought this would be a good start.

pedrocr commented 6 years ago

Nevertheless, I don't want my application (which uses rawloader) to just exit when the user tries to load an invalid file.

This won't happen. The default rawloader behavior is to catch the panic and issue an error. Your app will be fine.

johannesvollmer commented 6 years ago

Oh, I did not realize that.

Why did you choose to catch a panic instead of using Results? I thought that was considered un-idiomatic.

pedrocr commented 6 years ago

Here's the code that does that:

https://github.com/pedrocr/rawloader/blob/434626e5cda12280ba06d59fcca4c438064b30b9/src/decoders/mod.rs#L444-L456

This was an explicit design decision. Raw formats have all sorts of corner cases that would require sprinkling the code with a bunch of test cases and fuzzing the library extensively to catch them all. Instead we take advantage of rust features and just catch panic at the library boundary. It's not a very common solution but it works in this case where a file will either decode or fail. The alternative is much too cumbersome. I've tried it in C++ and it's a mess.

johannesvollmer commented 6 years ago

Sounds reasonable. Thanks for the explanation :+1: