log: rename LogPanicAndExit to HandlePanic #3642
Conversation
|
a discussion (no related file): Regarding the old one, at the call site: the reader would always think "Why am I logging a panic, I might not have one?", "Why am I exiting the application after finishing this goroutine?". Of course, this isn't what happens, but this is what the naming suggested. Regarding the new one, at the call site: similar questions pop up. "Why am I panicking when finishing this goroutine?", "Why am I exiting?". I think it's better to not hint at what is happening. So something like: It's more vague ("How is it handled?"), but vague is better than misleading. It will also free us from the name implying any contracts at the call site, so we can now plug handlers that do not kill a unit test when a goroutine in there panics (which is almost never what the programmer wants, since it black holes valuable test output). |
57dcda9 to
633b61b
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 69 files reviewed, 1 unresolved discussion (waiting on @scrye)
a discussion (no related file):
Previously, scrye (Sergiu Costea) wrote…
It feels like both the old and the new name are misleading.
Regarding the old one, at the call site:
defer log.LogPanicAndExit()the reader would always think "Why am I logging a panic, I might not have one?", "Why am I exiting the application after finishing this goroutine?". Of course, this isn't what happens, but this is what the naming suggested.
Regarding the new one, at the call site:
defer log.PanicAndExit()similar questions pop up. "Why am I panicking when finishing this goroutine?", "Why am I exiting?".
I think it's better to not hint at what is happening. So something like:
defer log.HandlePanic()It's more vague ("How is it handled?"), but vague is better than misleading.
It will also free us from the name implying any contracts at the call site, so we can now plug handlers that do not kill a unit test when a goroutine in there panics (which is almost never what the programmer wants, since it black holes valuable test output).
I do like that, and I made the change.
One thing that is unclear, in the past I remember that if we had omitted the log.logPanicAndExit there was a linter complaining, which seems not to work now. Anybody knows what/where?
There was a problem hiding this comment.
Reviewed 69 of 69 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @karampok)
a discussion (no related file):
Previously, karampok (Konstantinos) wrote…
I do like that, and I made the change.
One thing that is unclear, in the past I remember that if we had omitted the log.logPanicAndExit there was a linter complaining, which seems not to work now. Anybody know what/where?
I've filed https://github.com/Anapaya/scion/issues/2056 because something feels off, I also was unable to trigger the error.
go/lint/log.go, line 47 at r2 (raw file):
case *ast.FuncLit: if !checkFuncLit(pass, f) { pass.Reportf(f.Pos(), "First statement should be 'defer LogHandlePanic()")
s/LogHandlePanic/HandlePanic
go/lint/log.go, line 67 at r2 (raw file):
if pkgNameSave(pass) == "log" { ident, ok := deferStmt.Call.Fun.(*ast.Ident) if !ok || ident.Name != "LogHandlePanic" {
Same as above.
go/lint/log.go, line 72 at r2 (raw file):
} else { callSel, ok := deferStmt.Call.Fun.(*ast.SelectorExpr) if !ok || callSel.Sel.Name != "LogHandlePanic" {
Same as above.
2bea61d to
a77f14b
Compare
There was a problem hiding this comment.
Reviewable status: 32 of 64 files reviewed, 4 unresolved discussions (waiting on @scrye)
a discussion (no related file):
Previously, scrye (Sergiu Costea) wrote…
I've filed https://github.com/Anapaya/scion/issues/2056 because something feels off, I also was unable to trigger the error.
ok
go/lint/log.go, line 47 at r2 (raw file):
Previously, scrye (Sergiu Costea) wrote…
s/LogHandlePanic/HandlePanic
Done.
go/lint/log.go, line 67 at r2 (raw file):
Previously, scrye (Sergiu Costea) wrote…
Same as above.
Done.
go/lint/log.go, line 72 at r2 (raw file):
Previously, scrye (Sergiu Costea) wrote…
Same as above.
Done.
There was a problem hiding this comment.
Reviewed 39 of 39 files at r3.
Reviewable status:complete! all files reviewed, all discussions resolved
|
a discussion (no related file): |
a77f14b to
3da979d
Compare
There was a problem hiding this comment.
Reviewable status: 59 of 64 files reviewed, all discussions resolved (waiting on @scrye)
a discussion (no related file):
Previously, scrye (Sergiu Costea) wrote…
When merging, please make sure the final commit message refers to
log.HandlePanic, and not the older message.
the commit/PR message should be ok
According to the linter ``` func name will be used as log.LogPanicAndExit by other packages, and that stutters; consider calling this PanicAndExit ``` Contributes scionproto#3592
3da979d to
46bdd12
Compare
There was a problem hiding this comment.
Reviewed 5 of 7 files at r4.
Reviewable status:
According to the linter ``` func name will be used as log.LogPanicAndExit by other packages, and that stutters; consider calling this PanicAndExit ``` The name HandlePanic was picked. Contributes scionproto#3592
According to the linter
Contributes #3592
This change is