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

Better error Handling #67

Closed
wants to merge 30 commits into from

Conversation

Monirzadeh
Copy link
Contributor

@Monirzadeh Monirzadeh commented Aug 9, 2023

This Pull request try to remove panic from the code.
it should finally should
close #49
close #63

TODO:

  • Remove all panic [31 / 31]
  • Update unit test
  • fix macos error
    *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.

@bmaupin
Copy link
Owner

bmaupin commented Aug 9, 2023

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 panic with error codes.

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!

@Monirzadeh
Copy link
Contributor Author

OK.
thanks for feedback so i continue this approach and working on a better alternative for error package.
if it is not a problem i will ping you here to review this in small chunk. or if you set yourself as reviewer on this PR i can Request review with that.

@fmartingr
Copy link
Contributor

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.

We have this due in shiori ourselves. In order to wrap errors (the "new" way) you can use the %w denominator when creating an error: fmt.Errorf("error 1: %w", err)

@Monirzadeh
Copy link
Contributor Author

Monirzadeh commented Aug 10, 2023

@bmaupin remain 4 panic in write.go can you explain mere about them?

@bmaupin
Copy link
Owner

bmaupin commented Aug 10, 2023

@bmaupin remain 4 panic in write.go can you explain mere about them?

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.

@coveralls
Copy link

coveralls commented Aug 18, 2023

Coverage Status

coverage: 87.105% (-0.9%) from 88.0% when pulling ca5c88f on Monirzadeh:better-error-handling into 45f45a4 on bmaupin:main.

@Monirzadeh
Copy link
Contributor Author

@bmaupin because you want small change do you have any problem with new change?

@Monirzadeh Monirzadeh marked this pull request as ready for review August 19, 2023 10:45
@Monirzadeh
Copy link
Contributor Author

@bmaupin can we have an new release after this PR merge?

@fmartingr
Copy link
Contributor

@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.

@Monirzadeh
Copy link
Contributor Author

yes i do same things in go-shiori/shiori@49e8ba8 some bug detected that should be solve too :)

Monirzadeh and others added 7 commits August 29, 2023 15:17
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]>
@Monirzadeh Monirzadeh closed this by deleting the head repository Sep 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants