Skip to content

Use options pattern instead of config#450

Draft
philipch07 wants to merge 1 commit intomasterfrom
pch07/config-to-options
Draft

Use options pattern instead of config#450
philipch07 wants to merge 1 commit intomasterfrom
pch07/config-to-options

Conversation

@philipch07
Copy link
Contributor

@philipch07 philipch07 commented Dec 28, 2025

Description

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

Reference issue

Resolves #446

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

Codecov Report

❌ Patch coverage is 51.75097% with 124 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.54%. Comparing base (473ed5a) to head (f71b4fd).

Files with missing lines Patch % Lines
association_options.go 8.82% 61 Missing and 1 partial ⚠️
association.go 77.91% 19 Missing and 17 partials ⚠️
association_rack_options.go 0.00% 26 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #450      +/-   ##
==========================================
- Coverage   83.86%   81.54%   -2.33%     
==========================================
  Files          52       54       +2     
  Lines        4060     4258     +198     
==========================================
+ Hits         3405     3472      +67     
- Misses        479      590     +111     
- Partials      176      196      +20     
Flag Coverage Δ
go 81.54% <51.75%> (-2.33%) ⬇️

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
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, 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
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 2 times, most recently from ef25bcb to 5cdf355 Compare January 23, 2026 22:24
@philipch07
Copy link
Contributor Author

philipch07 commented Jan 23, 2026

^ 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.

@philipch07 philipch07 force-pushed the pch07/config-to-options branch 3 times, most recently from 51703bb to 527f759 Compare January 25, 2026 01:36
@philipch07 philipch07 force-pushed the pch07/config-to-options branch from 527f759 to f71b4fd Compare January 25, 2026 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update this library to use options API

2 participants