-
Notifications
You must be signed in to change notification settings - Fork 51
Conversation
Oh wow, thanks for starting this! This is much needed. I like your approach of doing it a little bit at a time. I took a quick look, and yes, my general thought was basically to replace all of the usages of For now I only have one comment, regarding https://github.com/pkg/errors ; this package is no longer maintained and the GitHub repo has been archived. From what I can tell, it looks like it wraps errors to provide context, which sounds great. But since that package isn't maintained, I'm hesitant to add it to this project. You can see at this issue that lots of other projects are abandoning it: pkg/errors#245 I think it would be best to either find an alternative or just leave it out. In theory if the error messages are descriptive enough, maybe we don't need it? This library is simple enough that maybe it could manage without. For instance, it looks like you're essentially wrapping errors yourself (without this library) when you do things like this: return nil, fmt.Errorf("Can't create NewEpub: %s", err) I'll try to take a closer look at this PR when I get a chance. Right now everything's blocked by the broken tests so I'd like to get #66 merged before merging anything else. Thanks! |
OK. |
We have this due in shiori ourselves. In order to wrap errors (the "new" way) you can use the |
@bmaupin remain 4 |
It looks like the first one will get called if there's an error when the file being added to the zip file gets closed. It looks like the others will get called if there's an error when the zip file writer is being closed due to a previous error in the process of writing the zip file. |
@bmaupin because you want small change do you have any problem with new change? |
@bmaupin can we have an new release after this PR merge? |
Remember that you can start working using the commit sha as version in the dependencies until this PR is merged and released. Once the release is made we can point it towards the proper release. |
yes i do same things in go-shiori/shiori@49e8ba8 some bug detected that should be solve too :) |
Co-authored-by: Felipe Martin <[email protected]>
Co-authored-by: Felipe Martin <[email protected]>
Co-authored-by: Felipe Martin <[email protected]>
Co-authored-by: Felipe Martin <[email protected]>
Co-authored-by: Felipe Martin <[email protected]>
Co-authored-by: Felipe Martin <[email protected]>
Co-authored-by: Felipe Martin <[email protected]>
Co-authored-by: Felipe Martin <[email protected]>
Co-authored-by: Felipe Martin <[email protected]>
Co-authored-by: Felipe Martin <[email protected]>
Co-authored-by: Felipe Martin <[email protected]>
This Pull request try to remove
panic
from the code.it should finally should
close #49
close #63
TODO:
*this todo list will update in future
@bmaupin it is not complete yet. I want remove all
panic
form code step by step. please review current state so i can understand your thought about this.i will remove more unneeded panic from code in future.