refactor: simplify NewConfig signature, add Configuration.Close#305
Open
meling wants to merge 5 commits intofeature/297/with-server-remove-server-newconfigfrom
Open
refactor: simplify NewConfig signature, add Configuration.Close#305meling wants to merge 5 commits intofeature/297/with-server-remove-server-newconfigfrom
meling wants to merge 5 commits intofeature/297/with-server-remove-server-newconfigfrom
Conversation
Renames the exported option type from ManagerOption to DialOption, aligning with gRPC conventions (grpc.DialOption) and the existing ServerOption / CallOption naming pattern. A deprecated type alias (type ManagerOption = DialOption) is added temporarily so that existing generated *_gorums.pb.go files continue to compile until they are regenerated in a follow-up commit. Closes #295. Related to #294.
Renames the exported Manager type to outboundManager and the exported NewManager constructor to newOutboundManager, hiding the connection-pool type from the public API. A deprecated type alias (type Manager = outboundManager) and a deprecated NewManager wrapper are added temporarily to keep existing callers and generated *_gorums.pb.go files compilable until they are updated in follow-up commits. Internal test helpers (TestManager, getOrCreateManager) and testopts are updated to reference *outboundManager directly. Closes #296. Related to #294.
Exports gorums.WithServer(srv *Server) DialOption as the canonical way to wire a Server as the back-channel handler when creating a client Configuration. This replaces the internal withRequestHandler call that was previously hidden inside (*Server).NewConfig. (*Server).NewConfig is removed; callers that used it, e.g. system_test.go, are updated to use gorums.NewConfig with gorums.WithServer(srv). Closes #297. Related to #294.
Changes NewConfig(opts ...Option) to NewConfig(nodes NodeListOption, opts ...DialOption), making the required NodeListOption explicit and aligning with standard Go API conventions (required args first). Additional changes: - Add Configuration.Close() error for closing connections without accessing the internal Manager directly. - Add unexported Configuration.mgr() used by Extend and Add internally. - Deprecate Configuration.Manager() in favor of Configuration.Close(). - Deprecate NewConfiguration; callers should use NewConfig instead. - Remove Manager, NewManager, NewConfiguration from generated code template; update NewConfig wrapper to the new signature. - Remove Manager from the reservedIdents list. NOTE: Existing generated files still call the old gorums.NewConfig signature and will fail to compile until regenerated in commit 6. Closes #298. Related to #294.
Changes NewConfig(opts ...Option) to NewConfig(nodes NodeListOption, opts ...DialOption), making the required NodeListOption explicit and aligning with standard Go API conventions (required args first). Additional changes: - Add Configuration.Close() error for closing connections without accessing the internal Manager directly. - Add unexported Configuration.mgr() used by Extend and Add internally. - Deprecate Configuration.Manager() in favor of Configuration.Close(). - Deprecate NewConfiguration; callers should use NewConfig instead. - Remove Manager, NewManager, NewConfiguration from generated code template; update NewConfig wrapper to the new signature. - Remove Manager from the reservedIdents list. NOTE: Existing generated files still call the old gorums.NewConfig signature and will fail to compile until regenerated in commit 6. Closes #298. Related to #294.
Contributor
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Go |  |
Mar 25, 2026 7:56a.m. | Review ↗ |
| Shell | |
Mar 25, 2026 7:56a.m. | Review ↗ |
meling
force-pushed
the
feature/297/with-server-remove-server-newconfig
branch
from
7a8369c to
dfa23d4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Previously, creating a configuration required two separate steps: create a
*Manager, then callNewConfiguration(mgr, nodes).This simplifies the API to a single constructor:
The manager is created internally and owned by the configuration.
Configuration.Close()is added to close all node connections and release resources.Changes
NewConfig(nodes NodeListOption, opts ...DialOption) (Configuration, error)— new simplified constructorConfiguration.Close() error— closes the owning manager and all node connectionsRelated
Part of #294
Closes #298