Skip to content
This repository has been archived by the owner on Aug 29, 2023. It is now read-only.

Better error handling #49

Open
owulveryck opened this issue Sep 15, 2021 · 1 comment
Open

Better error handling #49

owulveryck opened this issue Sep 15, 2021 · 1 comment

Comments

@owulveryck
Copy link
Collaborator

Hi!

This library is using extensively the panic function. I guess that, in most case, we should raise an error instead of panicing.

For example, I am using this library in a tool that fetches article on the web, apply a reading mode and create an epub.
Depending on the HTML structure, some stuffs are failing (bad src tags for images for examples, cf #45 ).
We would benefit from a proper error handling. The drawback is that it would change the API.

for example

func (e *Epub) SetCover(internalImagePath string, internalCSSPath string) {
...
}

could return an error if it cannot set the cover:

func (e *Epub) SetCover(internalImagePath string, internalCSSPath string) error {
...
}

I can take the action to post a PR for discussion if you want.

@bmaupin
Copy link
Owner

bmaupin commented Sep 15, 2021

Great feedback!

If I remember correctly I used panic when I ran into errors that should never happen or that were unrecoverable. But it always felt a bit weird and you're right that it's probably a bit extreme.

Would there still be some situations where panic would be warranted? For example, if there was an error creating a temporary file/directory, or an error closing a file? Or would it still be better to return an error in those situations?

I guess now I'm wondering if we should ever use panic? If a panic in the library results in a panic in the application using the library, then maybe we should never use it.

The drawback is that it would change the API

I'm not 100% against changing the API, and it's one of the reasons I've held off on v1.0.0 (even past that, we can always just bump the major version for breaking changes). I would just like to reserve breaking changes for when they're really needed, and this seems like a legitimate one.

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants