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

Add simple API extension to parse domain names with invalid characters #7

Closed
wants to merge 1 commit into from

Conversation

jonasbb
Copy link
Contributor

@jonasbb jonasbb commented Jul 29, 2017

This tries to address
#6

I tried to keep the API changes minimal, although I am not sure if the possibility to create Domain structs which are invalid is wanted, but it was the simplest way to implement it.

The validating could be made a bit more strict, by extending Domain::has_valid_syntax() to have a flag which ignores the regexset but performs the rest of the validation.

@rushmorem rushmorem closed this in 69a40f0 Jul 30, 2017
@rushmorem
Copy link
Owner

rushmorem commented Jul 30, 2017

Thanks for the pull request. I'm sorry I couldn't address this earlier due to other commitments. This is now fixed in v1.4.0.

I am not sure if the possibility to create Domain structs which are invalid is wanted

Indeed. Some people, myself included, are using this library to validate "registrable" domain names. As such, domain.root() must only return registrable domain names.

The validating could be made a bit more strict, by extending Domain::has_valid_syntax() to have a flag which ignores the regexset but performs the rest of the validation.

As per the RFC you linked to, the only restriction we should impose is making sure that DNS labels and total length are within the specified length. Other than that, libraries like this one must allow any string to pass. The new implementation for List::parse_dns_name tries to adhere to that.

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

2 participants