Skip to content

refactor: use Options struct in aibridgedserver.NewServer - #27200

Merged
ssncferreira merged 2 commits into
mainfrom
ssncf/aibridgedserver-options
Jul 16, 2026
Merged

refactor: use Options struct in aibridgedserver.NewServer#27200
ssncferreira merged 2 commits into
mainfrom
ssncf/aibridgedserver-options

Conversation

@ssncferreira

@ssncferreira ssncferreira commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Refactor aibridgedserver.NewServer to take an Options struct instead of a long list of positional arguments. Follow-up to review feedback in #27117 (comment)

@ssncferreira
ssncferreira force-pushed the ssncf/aibridgedserver-options branch 4 times, most recently from 5182a55 to f025c08 Compare July 14, 2026 08:39
@ssncferreira
ssncferreira force-pushed the ssncf/aibridgedserver-options branch from f025c08 to a26422a Compare July 14, 2026 08:43
@ssncferreira ssncferreira changed the title refactor: use Options struct in aibridgedserver.NewServe refactor: use Options struct in aibridgedserver.NewServer Jul 14, 2026
@ssncferreira
ssncferreira marked this pull request as ready for review July 14, 2026 08:44
) (*Server, error) {
eac := make(map[string]*externalauth.Config, len(externalAuthConfigs))
// Options carries the dependencies required to construct an aibridged Server.
type Options struct {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: when there are lot of members in struct I like to keep them sorted somehow for easier lookup, alphabetically or some combination of alphabetically + arbitrary "important" -> "boilerplate" ordering + optional domain grouping.

nit+: it is also nice to keep similar order when creating such struct.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that as well 👍 Initially, I didn't want to change a lot of the structure to keep it the same as before, but this looks nicer. Grouped into 1) Dependencies, 2) Configuration, 3) Common. Addressed in 34068e4

@ssncferreira
ssncferreira merged commit 101aee8 into main Jul 16, 2026
28 of 29 checks passed
@ssncferreira
ssncferreira deleted the ssncf/aibridgedserver-options branch July 16, 2026 15:09
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 16, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants