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

added .de region specific support #3

Merged
merged 12 commits into from
Apr 7, 2014
Merged

added .de region specific support #3

merged 12 commits into from
Apr 7, 2014

Conversation

ckoepp
Copy link
Contributor

@ckoepp ckoepp commented Feb 18, 2014

You seem to query whois.denic.de for the .de TLD. Unfortunately no data is received by your module as you're lacking the -T dn,ace -C US-ASCII string before the actually domain to query.

You can easily check the behaviour through
echo "-T dn,ace -C US-ASCII test.de" | nc whois.denic.de 43 and echo "test.de" | nc whois.denic.de 43

Added domain test.de to test-cases as this very domain is unlikely to change over time due to the state-sponsored organisation behind.

Test-cases are green and I guess there is no need to print the whole output there considered the case that this change is a quite trivial one. I do hope you agree 😄

@joepie91
Copy link
Owner

It seems good to go - the requirement of test.py output is primarily to make sure that people have tested it, which seems to be the case here :)

I do have one question before merging, though: is all WHOIS data for the entire .de TLD handled by whois.denic.de, or is there a possibility of a redirect to another WHOIS server? If redirects are a possibility, it should probably only apply those flags when querying whois.denic.de in particular, and not for everything ending in .de (similar to how the language flag is handled for .jp).

I also notice that this doesn't yet parse registrant data for whois.denic.de - while this is of course not required for this pull request, I'll make sure to implement that later (unless you were planning on doing so and submitting a pull request, in which case I'll avoid duplication of effort).

@ckoepp
Copy link
Contributor Author

ckoepp commented Feb 18, 2014

That's a good question actually...

I couldn't find any standard for such unusual behaviour to be honest. But since de.whois-servers.net is also pointing to 81.91.170.6 (= whois.denic.de) it may be safer to check for those names additionally.

Refererer services like whois.iana.org don't accept a request like this:

echo "-T dn,ace -C US-ASCII test.de" | nc whois.iana.org 43 versus echo "test.de" | nc whois.iana.org 43

So yes, there should be check for the two domains pointing to the whois DeNIC service. Although there may be more domains pointing to this very IP, but I guess those are quite exotic ones, right? So a simple check should do it.

@ckoepp
Copy link
Contributor Author

ckoepp commented Feb 18, 2014

...I also added an simple rfc3490 encoding feature (with two tests and documentation).

Have a look - although quite uncommon, some domains of local businesses are using it.

@joepie91
Copy link
Owner

joepie91 commented Mar 2, 2014

Whoops, sorry for not responding earlier! I've been very busy for the past two weeks...

Could you add the test output after your .de parsing changes? I should also add that if you name the matching groups street1, street2 etc. they will be combined into a single newline-delimited address :)

@ckoepp
Copy link
Contributor Author

ckoepp commented Mar 2, 2014

No problem about the delay.

You can find an example for the .de parsing in test/target_default/test.de or test/target_normalized/test.de. I also added the update of two domains as their name-servers changed.

Maybe you like to do an automated testing with travis-ci and use mocked data for simulating the whois services. By doing so every single pull request will be tested and the result is visible within seconds for everybody. Also no changed in whois-records are to break the tests (like it is the case currently).

To be honest I find it kind of annoying to copy'n'paste the whole bunch of testing output here 😄
It's working and can be verified by checking out my fork.

The street-solution is not ideal but resilient. Because I have no idea what the maximum amount of Address fields is. The only thing I found is that the street is always the last one of them as those others contain stuff like "c/o Sombody" or just a copy of the Organisation field. So I left it like this for now - but one can change that of course and try to get out more information :)

joepie91 added a commit that referenced this pull request Apr 7, 2014
@joepie91 joepie91 merged commit cfe36ed into joepie91:master Apr 7, 2014
@joepie91
Copy link
Owner

joepie91 commented Apr 7, 2014

After some more delays, I've finally gotten around to testing your pull request. It caused an error in Python 3.3 (which is supported since a few days ago after #4, and now tested through Tox), but this just turned out to be spaces as whitespace in your code, where the rest of the code uses tabs - so I've gone ahead and fixed that.

I've manually merged your pull request - it was targeted at master rather than develop, and the new version is now live on PyPI. Thanks for your help :)

Sir-Fenrir referenced this pull request in Sir-Fenrir/whois-oracle Jun 17, 2016
…468-detect-requests-exceeded to develop

* commit 'b0f201e9797179e61c31076009d9750d1ee4253e':
  ENH: Wording in a comment
  REF: Removed the fix for parsing empty responses and moved it to the whois application that uses this REF: Processed Wytse his comments
  REF: Renamed whois_response.py to raw_whois_response.py REF: Made timeout an argument, but with a default value
  REF: Increased default cool down length from 1 second to 2 REF: Extracted the starting of a cool down to a separate method ADD: Can now check whether a rate limit has been exceeded or not FIX: If there are now results at all, parse.py simply returns an empty dictionary instead of crashing
  ADD: A holder for WHOIS responses. It contains information about the success of the retrieval.
viliam-segeda referenced this pull request in viliam-segeda/pythonwhois-alt Aug 24, 2020
gthess pushed a commit to internetstandards/python-whois that referenced this pull request Apr 9, 2021
Ignore everything but the registrar from the whois data.
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.

2 participants