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

recover example #344

Closed
wants to merge 2 commits into from
Closed

recover example #344

wants to merge 2 commits into from

Conversation

kilaka
Copy link

@kilaka kilaka commented Feb 15, 2021

No description provided.

)

func main() {

Copy link
Collaborator

Choose a reason for hiding this comment

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

There aren't many comments in this sample. Is this intentional?

Copy link
Author

Choose a reason for hiding this comment

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

Added some comments in 47eed5e.
Coming from Java's "exceptions world", the recover seems a bit trivial, so I'm not sure what more info should I add.

recoverFromCustomPanic(-1)
}

func recoverFromBuiltInPanic(i int) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if having two almost identical functions for builtin and custom panics is really useful here.

All things being equal, the sample should be as small as possible. The goal here is to remember (or copy-paste) the proper invocation of recover inside a defer, not explore different ways in which Go can panic.

A simple "custom" example should suffice. The panic example has a panic, the recover example should just recover from the same panic.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed as you requested in 47eed5e.
Thanks for the feedback.

Copy link
Author

Choose a reason for hiding this comment

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

But I didn't use the same panic, as I think it's important to see that the panic might happen in a "lower" function call.
If it's critical I can change to align with the panic example. Or perhaps align the panic example?
Whatever you prefer...

@eliben eliben requested a review from mmcgrana February 15, 2021 14:17
@kilaka
Copy link
Author

kilaka commented Feb 15, 2021

Sorry guys but I'm still stuck with two problems which I hope you know how to fix quickly:

  1. Some strange auto-generated chars, which changed from \x3D to \u003D, causing a lot of files to look as if they were changed, but actually were not. (Perhaps related to working on a Linux laptop?).
  2. Misalignment between explanation/comments and code, specifically the comment above the recover.
  3. tools/build failed but succeeds locally: https://github.com/mmcgrana/gobyexample/pull/344/checks?check_run_id=1904178061

@eliben
Copy link
Collaborator

eliben commented Feb 15, 2021

IMHO the example should be even simpler, something like (with appropriate comments):

func mayPanic() {
	panic("a problem")
}

func main() {
	defer func() {
		if r := recover(); r != nil {
			fmt.Println("Recovered. Error:\n", r)
		}
	}()

	mayPanic()
	fmt.Println("After mayPanic()")
}

The main idea is not necessarily to teach all the ways in which panic/recover works, but to provide a small copy-pasteable example folks can use if they don't exactly remember the syntax/mechanics. I agree that it's useful to place the potential panic in a function, but the recover can be in main, and there's no need for any additional logic that's not specifically required for the panic->recover chain to work.

But this is just my opinion :-) I'll leave it to @mmcgrana to decide.

@eliben
Copy link
Collaborator

eliben commented Feb 15, 2021

For backlinking, this PR is related to issue #46

@eliben
Copy link
Collaborator

eliben commented Mar 18, 2021

@kilaka do you intend to keep working on this?

@kilaka
Copy link
Author

kilaka commented Mar 18, 2021

@eliben , not soon. But I thought we're waiting for @mmcgrana 's feedback...

@kilaka do you intend to keep working on this?

@mmcgrana
Copy link
Owner

mmcgrana commented May 2, 2021

@kilaka I think your example is in the right direction. I agree the example should be as simple as possible, @eliben's suggestion there seems plausible. I'd also want some brief comments about when/why one uses recover, especially since it is IIUC an unusual tool for application-level programers. See e.g. the Atomic Counter vs. Mutex vs. Stateful Goroutine pages for examples of lightweight when/why discussion. I've never used recover in my entire life as far as I know, so it's not easy for me to suggest particular comments, sorry 😬

@mmcgrana mmcgrana assigned kilaka and unassigned mmcgrana May 2, 2021
@eliben eliben closed this in 91c8cee Sep 1, 2021
@eliben
Copy link
Collaborator

eliben commented Sep 1, 2021

Thanks for discussion. I ended up adding a simplified example (see linked commit). Please open an issue/PR if you have questions or suggestions about the new example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants