-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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.
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
[confignet] Change Transport
from string
to TransportType
#9385
[confignet] Change Transport
from string
to TransportType
#9385
Conversation
Transport
from string
to TransportType
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9385 +/- ##
=======================================
Coverage 90.96% 90.97%
=======================================
Files 353 353
Lines 18626 18640 +14
=======================================
+ Hits 16943 16957 +14
Misses 1356 1356
Partials 327 327 ☔ View full report in Codecov by Sentry. |
6408fde
to
3c751a0
Compare
afac785
to
ac155b6
Compare
ac155b6
to
68f1f55
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this PR should not make any changes that affect end users like changing the struct tags related to this.
We may consider doing the breaking change on a separate PR (personally I don't think it is worth it), but since we can consider the changes separately, I think we should keep each change on a separate PR
Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
@bogdandrutu This is waiting on your review |
All tests are now passing except Contrib, which I'll fixed after this is merged should we choose to go forward with this breaking change approach. |
Description:
Changes
Transport
from astring
to a newTransportType
. ImplementsUnmarshalText
forTransportType
to enforce values.This PR may be too much - it introduces a breaking change a lot of new public APIs that may not be worth it for such a small module. If we don't like the surface area this creates or the breaking change, but we still want to enforce transport type values, I think implementing
Validate
keeps the API footprint smaller and isn't breaking.Link to tracking Issue:
Closes #9364
Documentation:
Added godoc comments