Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #786 +/- ##
==========================================
- Coverage 81.72% 81.65% -0.07%
==========================================
Files 108 109 +1
Lines 6062 6384 +322
==========================================
+ Hits 4954 5213 +259
- Misses 712 771 +59
- Partials 396 400 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fdb421a to
2877f5f
Compare
|
This is awesome @JoTurk , thank you for taking the time to do this! |
|
Okay i reviewed this few times and i think it's consistent with our other options patters, I'll add more coverage before merge but i think this is ready for review. |
adrianosela
left a comment
There was a problem hiding this comment.
This is amazing - 🚢 it!
2877f5f to
0a5b311
Compare
| OnConnectionAttempt: c.onConnectionAttempt, | ||
| } | ||
|
|
||
| if len(c.certificates) > 0 { |
There was a problem hiding this comment.
I just noticed that we are defensively copying twice. Not a big deal - but we don't need to AFAICT. (we are doing a copy when setting the option on dtlsConfig, so no need to copy again here)
cc:// @JoTurk
There was a problem hiding this comment.
I think we should remove the other copy, and make it copy for the old API, i'll make a PR for more cleanup and to update the tests. hopefully we can remove the other API sooner so we don't have to maintain both.
Description
(not ready for review, I'll double review this when i wake up).
This introduces options patterns to DTLS. it's a bit tricky because dtls options are a bit complex:
WithOptionshelpers, and in the next major release we're going to depreciate this suffix, and change the current constructors to options first.Updated all examples and the e2e test.
resolves: #765