Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Error Handling #8

Merged
merged 6 commits into from
Jan 13, 2024
Merged

Improve Error Handling #8

merged 6 commits into from
Jan 13, 2024

Conversation

kkoutsilis
Copy link
Owner

@kkoutsilis kkoutsilis commented Jan 11, 2024

In this PR we improve the error handling mechanism by using RunE that returns an error instead of Run , and delegating the error handling to the caller instead of log.Fatal() every time there is an error in the code.

@manavo
Copy link

manavo commented Jan 11, 2024

This should return the error instead of calling log.Fatalf: https://github.com/kkoutsilis/SSMG/blob/f1f066a1c99a591d0bff6cc707dc23699b29eb6f/cmd/root.go#L94C13-L94C13 (the sendEmails signature would need to change to return an error)

And then handle the error in the same way here: https://github.com/kkoutsilis/SSMG/pull/8/files#diff-ab967ab1a2f3a1b769106eeb7bfe892ef0e81d1d27811fa15be08e6749feee1fL152

@kkoutsilis
Copy link
Owner Author

kkoutsilis commented Jan 11, 2024

This should return the error instead of calling log.Fatalf: https://github.com/kkoutsilis/SSMG/blob/f1f066a1c99a591d0bff6cc707dc23699b29eb6f/cmd/root.go#L94C13-L94C13 (the sendEmails signature would need to change to return an error)

And then handle the error in the same way here: https://github.com/kkoutsilis/SSMG/pull/8/files#diff-ab967ab1a2f3a1b769106eeb7bfe892ef0e81d1d27811fa15be08e6749feee1fL152

Have a look now :) @manavo

@kkoutsilis kkoutsilis marked this pull request as ready for review January 11, 2024 21:31
@manavo
Copy link

manavo commented Jan 11, 2024

Looking better!

The only other consideration is that if one of the emails fails, the process exits rather than continuing.

Might be worth saving how many fail. If they all succeed, there's no error to return. If at least one succeeds, return an error which shows how many (and which ones?) failed?

@kkoutsilis
Copy link
Owner Author

Looking better!

The only other consideration is that if one of the emails fails, the process exits rather than continuing.

Might be worth saving how many fail. If they all succeed, there's no error to return. If at least one succeeds, return an error which shows how many (and which ones?) failed?

Yeah, this needs to be improved

@kkoutsilis
Copy link
Owner Author

Looking better!

The only other consideration is that if one of the emails fails, the process exits rather than continuing.

Might be worth saving how many fail. If they all succeed, there's no error to return. If at least one succeeds, return an error which shows how many (and which ones?) failed?

@manavo Added a temp solution that does not stop the execution if there is an error sending some email, will work on a better solution later on.

cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Show resolved Hide resolved
cmd/root.go Show resolved Hide resolved
@kkoutsilis kkoutsilis merged commit e372d7e into main Jan 13, 2024
2 checks passed
@kkoutsilis kkoutsilis deleted the improve-error-handling branch January 13, 2024 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants