proposal: extend netip.Addr support to ipv6 #792
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #792 +/- ##
=======================================
Coverage 92.56% 92.56%
=======================================
Files 23 23
Lines 5511 5515 +4
=======================================
+ Hits 5101 5105 +4
Misses 351 351
Partials 59 59 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Taking a second look at this, it appears it is possible to support dual-stack input by setting That doesn't seem very intuitive and could maybe warrant a doc fix? Personally I'd try making this a bit more explicit: the Also, it may be a good idea to switch to |
| case "ip": | ||
| if _, err := netip.ParseAddr(str); err != nil { | ||
| res.Add(path, str, validation.MsgExpectedRFCIPAddr) | ||
| } |
There was a problem hiding this comment.
@costela I love this! One concern I have is that ipv4 and ipv6 are actually in the JSON Schema spec but ip is not. See https://json-schema.org/understanding-json-schema/reference/type#ip-addresses.
I wonder if we should support all three? The two standard ones, and then ip for either one since Go supports it well. What do you think?
edit: just saw your other comment which is similar to mine, sorry 😂
896acfb to
b8026a4
Compare
|
Would be awesome if this PR could be merged |
netip.Addr supports parsing both ipv4 and ipv6 out of the box. So it is a bit surprising to have a
netip.Addrfield in an API only support ipv4 and no easy way to add "dual-stack" support for APIs.I propose to extend the support for
netip.Addradded in #396 and support ipv6 out of the box as well.formatdetection fornetip.Addrfields may be considered breaking, since existing APIs will start accepting IPv6 addresses and people may be counting on that limitation. I'd argue that it's worth it, but if not, we can remove that single change, while still adding support for theipdual-stack format, to be used explicitly.WDYT?