Conversation
|
@copilot review this PR. |
There was a problem hiding this comment.
Pull request overview
This PR appears to address issue #21 by improving runtime configuration of logging and slightly adjusting operational behavior/log verbosity in the P2P/protocol layer.
Changes:
- Add
common.InitLogger()to allow logger re-initialization after Viper config/env is loaded, with more flexibleloglevelparsing. - Adjust protocol startup/error handling and reduce periodic connection summary logging from Info to Debug.
- Update dependencies (notably
testify) and add an initialREADME.md.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/internal/protocol/host.go | Replaces panics with logging + os.Exit(1) during P2P host initialization. |
| src/internal/protocol/clock.go | Lowers connection summary log level from Info to Debug. |
| src/internal/common/logger.go | Introduces InitLogger() and enhances log level configuration parsing. |
| src/entry/cmd/root.go | Enables Viper env support and re-initializes logger after config setup. |
| src/go.mod | Bumps testify and adds indirect deps. |
| src/go.sum | Updates dependency checksums accordingly. |
| README.md | Adds initial project description. |
Comments suppressed due to low confidence (1)
src/internal/common/logger.go:43
zapLogger.Sync()is no longer called anywhere (the only call site is commented out). Without syncing on shutdown, buffered logs can be lost. Consider returning the built*zap.Logger(or exposing aSyncLogger()), and callingSync()frommain()/shutdown handling rather than ininit().
zapLogger, err := config.Build()
// defer func() { _ = zapLogger.Sync() }()
if err != nil {
panic(err)
}
Logger = zapLogger.Sugar()
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| host, err := newHost(ctx, seedInt, ds) | ||
| MyID = host.ID().String() | ||
| P2PNode = &host | ||
| if err != nil { | ||
| panic(err) | ||
| common.Logger.Error("Error while creating P2P node: ", err) | ||
| os.Exit(1) |
There was a problem hiding this comment.
host, err := newHost(...) is used (via host.ID().String()) before checking err. If newHost returns a nil host on error, this will panic before the error is logged/handled. Move the err check immediately after newHost and only set MyID/P2PNode when err == nil.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Xiaozhe Yao <xiaozhe.yaoi@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Xiaozhe Yao <xiaozhe.yaoi@gmail.com>
…24) * Initial plan * Fix error handling in GetP2PNode - check err before using host Co-authored-by: xzyaoi <10499297+xzyaoi@users.noreply.github.com> * Complete fix for error handling review feedback Co-authored-by: xzyaoi <10499297+xzyaoi@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: xzyaoi <10499297+xzyaoi@users.noreply.github.com>
No description provided.