Skip to content
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

feat: added mapper for regex #427

Closed
wants to merge 4 commits into from
Closed

Conversation

marsom
Copy link

@marsom marsom commented May 20, 2024

added mappers for:

  • netip.Addr
  • netip.Prefix
  • net.IP
  • net.IPNet
  • regexp.Regexp

Copy link
Owner

@alecthomas alecthomas left a comment

Choose a reason for hiding this comment

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

Love it, awesome idea.

@alecthomas
Copy link
Owner

For those linter failures, just add //nolint:dupl to each function.

@marsom
Copy link
Author

marsom commented May 21, 2024

Ok i will do it

mapper.go Outdated
Comment on lines 294 to 295
RegisterType(reflect.TypeOf(netip.Addr{}), netipAddrMapper()).
RegisterType(reflect.TypeOf(netip.Prefix{}), netipPrefixMapper()).
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry for the drive-by review, but are these two actually necessary? 🤔

they implement enconding.TextUnmarshaller and should work out of the box?

Copy link
Author

Choose a reason for hiding this comment

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

you are right, they work out of the box

Copy link
Author

Choose a reason for hiding this comment

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

i removed the netip mappers from the pr, and cleanup my tests.

I thought about also removing the net mappers, and only add the regex mapper:

  • this would guide users to use new new netip package instead of the net package

What do you think about that?

Copy link
Owner

Choose a reason for hiding this comment

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

Good question. Maybe both? I don't think there's any harm there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of leading people to netip. Especially since the package already has convenience code for converting back and forth to net.IP and friends.

But that's just my 2¢

@marsom marsom marked this pull request as draft May 21, 2024 20:52
@marsom marsom requested a review from costela May 21, 2024 20:59
Copy link
Contributor

@costela costela left a comment

Choose a reason for hiding this comment

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

As mentioned in the comment, I'd personally leave the "legacy" net.IP stuff out to nudge people toward netip but the PR looks great as is! 👍

@marsom marsom changed the title feat: added regex and net types: ip, ipnet, cidr feat: added mapper for regex May 23, 2024
@marsom marsom marked this pull request as ready for review May 23, 2024 19:56
Copy link
Contributor

@costela costela left a comment

Choose a reason for hiding this comment

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

🚀

mapper.go Outdated
@@ -733,6 +735,30 @@ func fileContentMapper(r *Registry) MapperFunc {
}
}

func regexMapper() MapperFunc {
Copy link
Owner

Choose a reason for hiding this comment

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

regexp.Regexp actually implements encoding.Text(Un)marshaler, so I don't think this is necessary either after all...

Copy link
Contributor

Choose a reason for hiding this comment

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

oh boy, you're right! 🤦
totally overlooked that too 🙈

Copy link
Author

Choose a reason for hiding this comment

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

sorry for consuming you time 🤦

i removed all mappers. we can close those this pr.

I added some regex tests in the last commit, if you have some time, maybe you can explain to me why the TestRegex,TestRegexSlice,TestRegexPointer are working but the TestRegexPointerSlice is not?

  • somehow i used to always use *regex.Regexp instead of regex.Regexp but the pointer version does not with the slice

@marsom marsom marked this pull request as draft May 24, 2024 09:09
@marsom marsom closed this May 27, 2024
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.

None yet

3 participants