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

Handling whois.Yoursrs.com empty last updated on #327

Merged
merged 5 commits into from Jun 5, 2014
Merged

Handling whois.Yoursrs.com empty last updated on #327

merged 5 commits into from Jun 5, 2014

Conversation

alexaitken
Copy link
Contributor

Problem:
There are some domains that have blank Last Updated On dates

Solution:
allow for blank dates.

Similar to #321

@weppos
Copy link
Owner

weppos commented Jun 4, 2014

Thanks for the patch @alexaitken. The change looks good, regarding Enom parser it makes sense for now to leave this change inside the parser itself instead of moving it to the ICANN parser.

Before merging the patch, I need you to make a small change in the test file. RSpec files are generated automatically, hence in order to write a test you need to provide an expected with the same name of the fixture file, then run rake spec:generate.

Here's an example
https://github.com/weppos/whois/blob/master/spec/fixtures/responses/whois.dot.cf/property_expires_on_blank.expected
https://github.com/weppos/whois/blob/master/spec/fixtures/responses/whois.dot.cf/property_expires_on_blank.txt

@weppos weppos self-assigned this Jun 4, 2014
@alexaitken
Copy link
Contributor Author

Will do. Also sorry about both patches being in this PR.

I just went over our errors again and I found one where class:BaseIcannCompliant is also hitting a blank updated on property. Looks like it would be worth moving the update_on empty check up a level.

@alexaitken
Copy link
Contributor Author

I have made the updates.

what do you think about moving the empty check up to the baseIcannCompliant parser?

@weppos
Copy link
Owner

weppos commented Jun 4, 2014

On Wed, Jun 4, 2014 at 4:17 PM, Alex Aitken notifications@github.com
wrote:

what do you think about moving the empty check up to the
baseIcannCompliant parser?

I'm against such proposal, unless a consistent number of registries under
the BaseIcannCompliant exposes such behavior.
At the moment 2 is a reasonably low use case.

Simone Carletti
Passionate programmer and dive instructor

http://simonecarletti.com/
Twitter: @weppos https://twitter.com/weppos

@alexaitken
Copy link
Contributor Author

Sounds good. If I see the error again I will find out which other registry is responding that way and fix in the isolated place.

weppos added a commit that referenced this pull request Jun 5, 2014
Handling whois.Yoursrs.com empty last updated on
@weppos weppos merged commit cbb3daa into weppos:master Jun 5, 2014
@weppos
Copy link
Owner

weppos commented Jun 5, 2014

Thanks for the patch!

weppos added a commit that referenced this pull request Jun 5, 2014
@alexaitken alexaitken deleted the yoursrs-empty-last-updated-on branch June 25, 2014 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants