refactor: simplify NewConfig signature, add Configuration.Close#305
Merged
refactor: simplify NewConfig signature, add Configuration.Close#305
Conversation
Contributor
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Go |  |
Mar 25, 2026 1:38p.m. | Review ↗ |
| Shell | |
Mar 25, 2026 1:38p.m. | Review ↗ |
meling
force-pushed
the
feature/297/with-server-remove-server-newconfig
branch
from
7a8369c to
dfa23d4
Compare
Base automatically changed from
feature/297/with-server-remove-server-newconfig
to
master
March 25, 2026 11:26
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.
meling
force-pushed
the
feature/298/simplify-newconfig-signature
branch
from
d5148e3 to
cad6f15
Compare
Contributor
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
meling
force-pushed
the
feature/298/simplify-newconfig-signature
branch
from
cad6f15 to
822cab8
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Previously, WithMetadata replaced o.metadata with the caller-supplied value. This clobbered node-id metadata set by withRequestHandler (via WithServer) when WithServer was applied before a user-provided WithMetadata option, breaking reverse-direction routing. Fix by using metadata.Join so that WithMetadata accumulates metadata instead of replacing it, consistent with the existing PerNodeMD and withRequestHandler patterns. Add TestWithMetadataJoinsInsteadOfOverwrites to cover the ordering case.
This was referenced Mar 25, 2026
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
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