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

docs(readme): add error-handling to example code #513

Merged
merged 1 commit into from
Sep 19, 2020

Conversation

dominicbarnes
Copy link
Contributor

@dominicbarnes dominicbarnes commented Sep 18, 2020

The current README includes quite a few code snippets that include no error-handling. If someone is trying to learn this library for the first time, it can make understanding why things fail much more difficult without first adding this error-handling to their own code.

It's just good hygiene in Go to handle errors, so I think our examples should reflect that, rather than try so hard to keep the code shorter.

The current README includes quite a few code snippets that include no
error-handling. If someone is trying to learn this library for the first
time, it can make understanding why things fail much more difficult
without first adding this error-handling to their own code.

It's just good hygiene in Go to handle errors, so I think our examples
should reflect that, rather than try so hard to keep the code shorter.
Copy link
Contributor

@achille-roussel achille-roussel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for improving the examples @dominicbarnes 👍

Copy link
Contributor

@stevevls stevevls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM. One philosophical question: should we use panic instead of log.Fatal? My understanding is that log.Fatal has generally fallen out of favor since it calls os.Exit and therefore does not execute deferred statements.

@dominicbarnes
Copy link
Contributor Author

@stevevls I'm open to either approach, just as long as we're consistent. I tend to lean towards this as it's likely closer to what a user would do in practice, aside from return err which wouldn't compile in these particular examples. That being said, I do see your point about panic and could be convinced to change this if others chime in (otherwise, will probably just merge as-is).

@dominicbarnes dominicbarnes merged commit 418f1fd into master Sep 19, 2020
@dominicbarnes dominicbarnes deleted the readme-error-handling branch September 19, 2020 07:24
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.

3 participants