Improves error reporting #996
Conversation
DavidSeptimus-Klotho
commented
Aug 16, 2024
•
DavidSeptimus-Klotho
commented
- In concise mode, errors are rendered cleanly at the end of the terminal output
- Silenced cobra error logging to avoid breaking our TUI output
- Added error detection for language host startup
- Added error messages to clarify issues with the klotho sdk installation
pkg/tui/log_core.go
Outdated
| @@ -47,7 +47,7 @@ func (c *LogCore) With(f []zapcore.Field) zapcore.Core { | |||
| if c.verbosity.CombineLogs() { | |||
| field.AddTo(nc.enc) | |||
| } | |||
| // else (if the field is the construct and we're not combining logs) don't add it to the encoder | |||
| // else (if the field is the construct and we're not combining errors) don't add it to the encoder | |||
There was a problem hiding this comment.
Combing logs is the correct word here. This describes the difference, for eg, between -v=2 and -v=3
pkg/tui/verbosity.go
Outdated
| // CombineLogs controls whether to show all errors commingled in the TUI. | ||
| // In other words, sorted by timestamp, not grouped by construct. |
There was a problem hiding this comment.
I don't think this comment is accurate - it's still just the logs comingled (uses program.Println) or grouped (uses tui.model).
There was a problem hiding this comment.
sigh Intellij refactoring strikes again.
| func NewServerState(log *zap.SugaredLogger) *ServerState { | ||
| return &ServerState{ | ||
| Log: log, | ||
| Done: make(chan any), |
There was a problem hiding this comment.
nit: chan any uses the size of a pointer whereas chan struct{} uses zero size. It also uses the types to indicate that it's just a signal, and that no meaningful information other than its completion can be derived from the channel. It's not a big deal, but just explaining why it was that previously.
DavidSeptimus-Klotho
force-pushed
the
language-host-errors
branch
from
48e50a3
to
9ce6af9
Compare
- In concise mode, errors are rendered cleanly at the end of the terminal output - Silenced cobra error logging to avoid breaking our TUI output - Added error detection for language host startup - Added error messages to clarify issues with the klotho sdk installation
DavidSeptimus-Klotho
force-pushed
the
language-host-errors
branch
from
9ce6af9
to
81149d2
Compare
| } | ||
|
|
||
| if f.Address != "" || f.Error != nil { |
There was a problem hiding this comment.
I think this condition is inverted - those conditions are when it is already closed.
There was a problem hiding this comment.
If one of those is not nil/empty, we close the channel. This is the only place we close the channel. What am I missing?
There was a problem hiding this comment.
Consider the 2nd time an Error or Address are encountered, this condition would be true the second time, resulting in a double close. Unless we're doing something to shut it down, but I don't think we are (and not sure we should for cases like Address -> Error).
There was a problem hiding this comment.
Just updated this change. Now, we gate the close behind an if-else-if, so it only calls one or the other. Once Either branch evaluates to true, we close the channel and any subsequent calls to write will just return immediately. So you only get either an error or an address, but never both.
Once an error or address is captured, ServerState.Write() should just do nothing on subsequent writes. In conjunction with if-else-if, it ensures that we only close the Done channel once.
