Copilot commented on code in PR #980:
URL: https://github.com/apache/dubbo-go-pixiu/pull/980#discussion_r3418562755
##########
admin/dao/impl/guest.go:
##########
@@ -87,5 +87,5 @@ func (d *GuestDao) Register(username, password string) error {
}
func (d *GuestDao) CheckLogin() {
- panic("implement me")
+ // TODO: implement login session validation
Review Comment:
`CheckLogin()` changed from failing fast (panic) to silently succeeding
(no-op). If this method is part of an auth/session validation path, this can
effectively bypass enforcement. Prefer failing closed until implemented (e.g.,
return an error and propagate it, or keep a hard failure/log+abort); if the
signature can’t change, consider explicitly signaling failure rather than doing
nothing.
##########
pkg/adapter/dubboregistry/registry/nacos/application_service_listener.go:
##########
@@ -66,7 +66,8 @@ func newNacosAppSrvListener(client
naming_client.INamingClient, adapterListener
}
func (l *appServiceListener) WatchAndHandle() {
- panic("implement me")
+ // TODO: implement WatchAndHandle for application-level service
discovery
+ logger.Warnf("appServiceListener: WatchAndHandle not implemented")
Review Comment:
Same concern as `serviceListener.WatchAndHandle()`: this will warn on every
call and may spam logs. Consider making it a no-op (or debug-only), or ensure
the warning is emitted only once while the TODO remains.
##########
pkg/adapter/dubboregistry/registry/nacos/service_listener.go:
##########
@@ -55,7 +55,8 @@ type serviceListener struct {
// WatchAndHandle todo WatchAndHandle is useless for service listener
func (z *serviceListener) WatchAndHandle() {
- panic("implement me")
+ // WatchAndHandle is not used by service listener; subscription uses
Callback instead.
+ logger.Warnf("serviceListener: WatchAndHandle not implemented")
Review Comment:
Logging a warning unconditionally every time `WatchAndHandle()` is called
can create noisy logs and alert fatigue if this method is invoked repeatedly
(or mistakenly by framework code). If this is intentionally unused, prefer a
no-op, a debug-level log, or guard the warning so it only logs once (e.g., via
`sync.Once`).
##########
pkg/cmd/gateway.go:
##########
@@ -133,8 +133,8 @@ func (d *DefaultDeployer) start() error {
}
func (d *DefaultDeployer) stop() error {
- // TODO implement me
- panic("implement me")
+ // TODO: implement graceful shutdown
+ return fmt.Errorf("stop: not implemented")
Review Comment:
`stop()` now always returns an error on every shutdown attempt. If callers
invoke `stop()` during normal lifecycle teardown, this will reliably surface as
a shutdown failure even when shutdown is otherwise clean. Consider making
`stop()` a no-op that returns `nil` until graceful shutdown is implemented (and
optionally log once), or implement the actual shutdown logic now.
##########
pkg/adapter/springcloud/servicediscovery/nacos/nacos.go:
##########
@@ -202,19 +202,19 @@ func (n *nacosServiceDiscovery) QueryAllServices()
([]servicediscovery.ServiceIn
}
func (n *nacosServiceDiscovery) Register() error {
- panic("implement me")
+ return fmt.Errorf("nacosServiceDiscovery: Register not implemented")
}
func (n *nacosServiceDiscovery) UnRegister() error {
- panic("implement me")
+ return fmt.Errorf("nacosServiceDiscovery: UnRegister not implemented")
}
func (n *nacosServiceDiscovery) Get(s string)
[]*servicediscovery.ServiceInstance {
- panic("implement me")
+ return nil
}
func (n *nacosServiceDiscovery) StartPeriodicalRefresh() error {
- panic("implement me")
+ return fmt.Errorf("nacosServiceDiscovery: StartPeriodicalRefresh not
implemented")
Review Comment:
`fmt.Errorf` is used with constant strings (no formatting). Prefer
`errors.New(...)` for clarity and to avoid unnecessary formatting overhead. If
the project uses a standard error type/wrapper (e.g., `perrors`), consider
using it consistently here too.
##########
pkg/common/extension/filter/filter.go:
##########
@@ -157,41 +157,41 @@ var (
// OnDecode empty implement
func (enf *EmptyNetworkFilter) OnDecode(data []byte) (any, int, error) {
- panic("OnDecode is not implemented")
+ return nil, 0, errors.New("EmptyNetworkFilter: OnDecode not
implemented")
}
// OnEncode empty implement
func (enf *EmptyNetworkFilter) OnEncode(p any) ([]byte, error) {
- panic("OnEncode is not implemented")
+ return nil, errors.New("EmptyNetworkFilter: OnEncode not implemented")
}
// OnData receive data from listener
func (enf *EmptyNetworkFilter) OnData(data any) (any, error) {
- panic("OnData is not implemented")
+ return nil, errors.New("EmptyNetworkFilter: OnData not implemented")
}
// OnTripleData empty implement
func (enf *EmptyNetworkFilter) OnTripleData(ctx context.Context, methodName
string, arguments []any) (any, error) {
- panic("OnTripleData is not implemented")
+ return nil, errors.New("EmptyNetworkFilter: OnTripleData not
implemented")
}
// OnUnaryRPC empty implement
func (enf *EmptyNetworkFilter) OnUnaryRPC(ctx context.Context, fullMethod
string, req any) (any, error) {
- panic("OnUnaryRPC is not implemented")
+ return nil, errors.New("EmptyNetworkFilter: OnUnaryRPC not implemented")
}
// OnStreamRPC empty implement
func (enf *EmptyNetworkFilter) OnStreamRPC(stream model.RPCStream, info
*model.RPCStreamInfo) error {
- panic("OnStreamRPC is not implemented")
+ return errors.New("EmptyNetworkFilter: OnStreamRPC not implemented")
}
// ServeHTTP empty implement
func (enf *EmptyNetworkFilter) ServeHTTP(w stdHttp.ResponseWriter, r
*stdHttp.Request) {
- panic("ServeHTTP is not implemented")
+ stdHttp.Error(w, "EmptyNetworkFilter: ServeHTTP not implemented",
stdHttp.StatusNotImplemented)
}
func (enf *EmptyNetworkFilter) Close() error {
- panic("Close is not implemented")
+ return nil
Review Comment:
`EmptyNetworkFilter` methods mostly return explicit “not implemented”
errors, but `Close()` now silently returns `nil`. If `Close()` is expected to
be meaningful for resource cleanup in this interface, this inconsistency can
hide incorrect usage. Either (a) return a similar “not implemented” error for
consistency, or (b) add a brief comment/doc explaining that `Close()` is
intentionally a no-op because there are no resources to release.
##########
pkg/adapter/springcloud/servicediscovery/nacos/nacos.go:
##########
@@ -202,19 +202,19 @@ func (n *nacosServiceDiscovery) QueryAllServices()
([]servicediscovery.ServiceIn
}
func (n *nacosServiceDiscovery) Register() error {
- panic("implement me")
+ return fmt.Errorf("nacosServiceDiscovery: Register not implemented")
Review Comment:
`fmt.Errorf` is used with constant strings (no formatting). Prefer
`errors.New(...)` for clarity and to avoid unnecessary formatting overhead. If
the project uses a standard error type/wrapper (e.g., `perrors`), consider
using it consistently here too.
##########
pkg/adapter/springcloud/servicediscovery/nacos/nacos.go:
##########
@@ -202,19 +202,19 @@ func (n *nacosServiceDiscovery) QueryAllServices()
([]servicediscovery.ServiceIn
}
func (n *nacosServiceDiscovery) Register() error {
- panic("implement me")
+ return fmt.Errorf("nacosServiceDiscovery: Register not implemented")
}
func (n *nacosServiceDiscovery) UnRegister() error {
- panic("implement me")
+ return fmt.Errorf("nacosServiceDiscovery: UnRegister not implemented")
Review Comment:
`fmt.Errorf` is used with constant strings (no formatting). Prefer
`errors.New(...)` for clarity and to avoid unnecessary formatting overhead. If
the project uses a standard error type/wrapper (e.g., `perrors`), consider
using it consistently here too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
