Skip to content

Use options pattern instead of config #450

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
philipch07 wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Use options pattern instead of config #450

philipch07 wants to merge 1 commit into from
+324 −43

Conversation

@philipch07
Copy link
Contributor

@philipch07 philipch07 commented Dec 28, 2025 •
edited
Loading

Description

Uses the options pattern instead of the config pattern, which is a necessary change before adding SNAP (#449).

Reference issue

Resolves #446

JoTurk
JoTurk reviewed Dec 28, 2025
View reviewed changes
Copy link
Member

@JoTurk JoTurk left a comment

Choose a reason for hiding this comment

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

yeah this is a good direction, we need to add more validations tho.

@philipch07 philipch07 force-pushed the pch07/config-to-options branch from f888d39 to 1192971 Compare December 28, 2025 18:23
@codecov
Copy link

codecov bot commented Dec 28, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 18.34862% with 89 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.00%. Comparing base (473ed5a) to head (5cdf355).

Files with missing lines Patch % Lines
association_options.go 0.00% 56 Missing ⚠️
association_rack_options.go 0.00% 26 Missing ⚠️
association.go 74.07% 7 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #450      +/-   ##
==========================================
- Coverage   83.86%   82.00%   -1.87%     
==========================================
  Files          52       54       +2     
  Lines        4060     4151      +91     
==========================================
- Hits         3405     3404       -1     
- Misses        479      570      +91     
- Partials      176      177       +1
Flag Coverage Δ
go 82.00% <18.34%> (-1.87%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@philipch07 philipch07 force-pushed the pch07/config-to-options branch 2 times, most recently from 124aeb4 to 32dc80b Compare December 28, 2025 19:22
JoTurk
JoTurk reviewed Dec 28, 2025
View reviewed changes
@philipch07 philipch07 force-pushed the pch07/config-to-options branch from 32dc80b to 8021a05 Compare December 28, 2025 20:24
@JoTurk JoTurk mentioned this pull request Dec 28, 2025
Draft
@philipch07 philipch07 force-pushed the pch07/config-to-options branch from 8021a05 to aa606bd Compare December 28, 2025 20:36
JoTurk
JoTurk reviewed Dec 30, 2025
View reviewed changes
Copy link
Member

@JoTurk JoTurk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

looks good, i think we'll just need to add test coverage and make newAssociationWithOptions public

@JoTurk
Copy link
Member

JoTurk commented Dec 30, 2025

^ mean making newAssociationWithOptions through new APIs to construct Client and Server.

@philipch07 philipch07 force-pushed the pch07/config-to-options branch from aa606bd to c1a345b Compare January 21, 2026 20:36
JoTurk
JoTurk reviewed Jan 22, 2026
View reviewed changes
Copy link
Member

@JoTurk JoTurk left a comment

Choose a reason for hiding this comment

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

looks good, just need to finish it, fix the lint and it's ready for merge.

@philipch07 philipch07 force-pushed the pch07/config-to-options branch from c1a345b to ef25bcb Compare January 23, 2026 21:59
@philipch07 philipch07 force-pushed the pch07/config-to-options branch from ef25bcb to 5cdf355 Compare January 23, 2026 22:24
@philipch07
Copy link
Contributor Author

philipch07 commented Jan 23, 2026 •
edited
Loading

^ mean making newAssociationWithOptions through new APIs to construct Client and Server.

@JoTurk I'm not sure if the client and server need to have specific settings for each beyond the existing options. Did you have something more specific in mind? I left a couple TODO's that you can ctrl+f in associations.go and association_options.go where I'm unsure about how to proceed.

Also looking into createAssociation(), it currently just takes Config but changing to ServerConfig or ClientConfig would imply to users that there are different configs that need to be set up, so we would probably have to be careful to specify whatever the differences are if we choose to split them.

@JoTurk
Copy link
Member

JoTurk commented Jan 24, 2026

@philipch07 In DTLS we have Option which is a union between client and server options. For SCTP we don't need this split now because our current options symmetric, but IMO it's nice to split them early before we eventually add dial/listen APIs in v2 and different options for client and server with overlapping options.

100% up to you, I support any choice you pick.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JoTurk JoTurk JoTurk left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Update this library to use options API

3 participants

@philipch07 @JoTurk