Skip to content
This repository was archived by the owner on Jan 18, 2023. It is now read-only.

Conversation

@a2ikm
Copy link
Contributor

@a2ikm a2ikm commented Jan 22, 2018

This PR adds supports of dot-separated addresses like xip.io.

@resmo
Copy link
Owner

resmo commented Jan 23, 2018

please rebase

@a2ikm
Copy link
Contributor Author

a2ikm commented Jan 23, 2018

@resmo
I rebased it now. Could you see it again?

if DEBUG:
log('%s is not a number' % part)
self.handle_self(qname)
return
Copy link
Owner

Choose a reason for hiding this comment

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

I would like to keep this code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@resmo
I got it. I reverted this change in 751ccb5.

Copy link

@Doqnach Doqnach left a comment

Choose a reason for hiding this comment

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

regex suggestion

src/nip.py Outdated
subdomain = qname[0:qname.find(self.domain) - 1]
# Split foo.bar.10-0-0-1 in parts
subdomain_parts = subdomain.split('.')
match = re.findall('^(?:.+\.)?(\d{1,3}[-.]\d{1,3}[-.]\d{1,3}[-.]\d{1,3})$', subdomain)
Copy link

Choose a reason for hiding this comment

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

I would suggest supporting either - or . but not both at the same time. This could be done using back-references: ^(?:.+\.)?(\d{1,3}([-.])\d{1,3}\2\d{1,3}\2\d{1,3})$ https://regex101.com/r/Tamzd5/2

Copy link
Owner

Choose a reason for hiding this comment

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

agreed!

Copy link
Owner

Choose a reason for hiding this comment

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

@Doqnach would you like to pick it up?

Copy link

Choose a reason for hiding this comment

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

I would love to, but I will be very honest in saying that I have no idea how to and neither do I have any way of testing the change before creating a PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Doqnach
Thank you, your suggestion is very reasonable. So I picked it in f0640ad.

a2ikm added 2 commits April 27, 2018 18:26
Match eather `-` or `.` but not both at the same time.
@a2ikm
Copy link
Contributor Author

a2ikm commented Apr 27, 2018

Thank you for your comments.
I'll squash the changes if they are ok 😃

@resmo resmo merged commit 2f7d368 into resmo:master May 13, 2018
@resmo
Copy link
Owner

resmo commented May 13, 2018

Thanks!

@a2ikm
Copy link
Contributor Author

a2ikm commented May 13, 2018

Thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants