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

Remove must from go-clr.go #3

Merged
merged 1 commit into from
Nov 10, 2022
Merged

Remove must from go-clr.go #3

merged 1 commit into from
Nov 10, 2022

Conversation

mec07
Copy link

@mec07 mec07 commented Oct 25, 2022

We really must remove must(err) error handling from go-clr.go because it can cause your entire program to bomb out without even the slightest chance of recovery. It makes this library incredibly unstable. If it goes wrong it should return an error, not log.Fatal(err). log.Fatal doesn't even allow defer statements to run! It's really bad.

I'm leaving it in the examples folder though as it doesn't cause any direct issues there.

Note: I've forked off your fork and I'm creating this PR for to it because I can see that you've made quite a few improvements upon the original repo. Thanks for taking the original and improving it so much!

@mec07
Copy link
Author

mec07 commented Oct 25, 2022

I'm tagging you, @Ne0nd0g, to bring this PR to your attention. Thanks a ton for improving so much upon the original repo! I'm quite excited about using this package and will probably want to push some more improvements as I start using it and learn more about it.

@Ne0nd0g
Copy link
Owner

Ne0nd0g commented Oct 25, 2022

@mec07 Really appreciate the PR and I agree we should be returning errors instead of exiting. I'll work on getting this merged in soon.

@mec07
Copy link
Author

mec07 commented Oct 25, 2022

@mec07 Really appreciate the PR and I agree we should be returning errors instead of exiting. I'll work on getting this merged in soon.

Thanks! 😄 I've tweeted you a question 😅 . Hopefully it's clear. Twitter isn't the best way to ask detailed questions!

@Ne0nd0g Ne0nd0g changed the base branch from master to dev November 10, 2022 23:13
@Ne0nd0g Ne0nd0g merged commit ce145f5 into Ne0nd0g:dev Nov 10, 2022
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