-
Notifications
You must be signed in to change notification settings - Fork 192
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
Small regexp fixes for Go compatibility #162
Conversation
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.
Thanks for the contributions HD. I've one small comment on the DNS change but other than that it looks great. It's a unfortunate that Golang doesn't support lookahead and lookbehind assertions.
@@ -304,7 +304,7 @@ | |||
<param pos="0" name="service.family" value="PowerDNS"/> | |||
<param pos="0" name="service.product" value="Recursor"/> | |||
</fingerprint> | |||
<fingerprint pattern="^PowerDNS Authoritative Server (\d\.[\d.]++(?:-rc\d)?) \(\w+@[\w.]+ built [\d\s]+\w*@[\w.-]*\)$"> | |||
<fingerprint pattern="^PowerDNS Authoritative Server (\d\.[\d\.]+(?:-rc\d)?) \(\w+@[\w.]+ built [\d\s]+\w*@[\w.-]*\)$"> |
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.
This character is valid in the context of this character class group, similar to the one at the end of the pattern. I've tested this both ways with an online regex tester for Golang but I haven't tested it natively with Golang. Unless you have a concern I'll revert this specific change.
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.
Thanks! This looked like a typo, since it was supposed to match "1.2.3" not "1.anything". Keeping it as "." (anything) works too, but it seemed like the wrong logic given the examples.
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.
Supposedly the 're2' go implementation supports negative look ahead, but it seemed faster to change these than switch to non-standard library given how rare they were used.
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.
Within the character class grouping []
, .
functions as a literal character .
instead of a wild card. I went ahead and verified this at https://regex101.com/r/cstrTL/1 to ensure that I didn't misremember and that it wasn't different in Golang. ^[.]$
matches .
but not f
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.
Oh, the "." in the end of the pattern has the same issue, it matches everything, not just period.
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.
It should not, see my comment just above your last comment. I updated it to be more specific and to provide a link. This probably didn't send an updated notice.
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.
Thank you, that was news to me.
Landed, thanks much HD. |
Awesome! Thank you for landing it. I should have a recog-go package available in the next few weeks (it works now, but is REALLY ugly). |
Sounds great! Also, your PR made me aware of the Golang lookahead/lookbehind limitations (in the native lib) which likely saved me tons of time on a project I was going to use to learn Golang so thanks for that too. |
This change corrects two typos and removes two negative look aheads, allowing these expressions to be parsed by the Go standard library 'regexp' engine.