New coza whois functionality #191

Closed
wants to merge 12 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

rorymckinley commented Nov 1, 2012

Uniforum (the .co.za registry) has introduced a new WHOIS service that provides significantly more functionality than the old one. This change removes the old WHOIS functionality and introduces a new parser.

Owner

weppos commented Nov 3, 2012

Hi @rorymckinley, thanks for your patch. I have a few comments about it before considering it ready for the merge. I'll post them inline.

+module Whois
+ class Record
+ class Parser
+ #--
@weppos

weppos Nov 3, 2012

Owner

Please remove the #-- and #++ boundaries because it forces to ignore the comment. Also, please add the class header.

# Parser for the whois.registry.za.net server.
#
# @note This parser is just a stub and provides only a few basic methods
#   to check for domain availability and get domain status.
#   Please consider to contribute implementing missing methods.
# 
# @see Whois::Record::Parser::Example
#   The Example parser for the list of all available methods.
#
+ end
+
+ property_supported :registrar do
+ if node(:registrar_id)
@weppos

weppos Nov 3, 2012

Owner

Else condition with nil is pointless. If the condition is not evaluated, the result is nil.
Also, you can condense everything using a block

node(:registrar_id) do
   # ...
end

The same principle applies to other methods.

+ end
+
+ property_supported :nameservers do
+ if registered?
@weppos

weppos Nov 3, 2012

Owner

Please don't bound the check to registered. You can write the same code as

Array.wrap(node(:nameservers)).map do |name|
  Record::Nameserver.new(:name => name)
end
@@ -0,0 +1,29 @@
+#available?
+ should: %s be_true
@weppos

weppos Nov 3, 2012

Owner

Please don't use RSpec-specific shortcuts. I'll probably try to convert the files into a meta-format easily parsable by other non-Ruby implementations.
See existing expected files for some hints.

Specifically, avoid

be_false -> == false
be_true -> == true
be_empty -> == []
be_nil -> == nil

Also, make sure to test for all methods, including unsupported methods (aka methods that raise a Whois::PropertyNotSupported error).

+ should: %s == "broccoliwafflesareawesome.co.za"
+
+#created_on
+ should: %s == Time.new(2012,3,27,nil,nil,nil,"+02:00")
@weppos

weppos Nov 3, 2012

Owner

Please use the form

Time.parse("2013-03-27")

Don't inject a timezone unless the timezone is explicitly included in the response.

+#expires_on
+ should: %s == Time.new(2013,3,27,nil,nil,nil,"+02:00")
+
+#disclaimer
@weppos

weppos Nov 3, 2012

Owner

Please make sure to use the same standard property order as all the other parsers. Whitespaces are also relevant.

#disclaimer
  ...


#domain
  ...

#domain_id
  ...


#status
  ...

#available?
  ...

#registered?
  ...

The same applies to the Ruby parser definition.

+ node(:disclaimer)
+ end
+
+ property_not_supported :domain_id
@weppos

weppos Nov 3, 2012

Owner

As for the tests, please define the properties in the same order as all the other parsers.

+
+ tokenizer :get_domain_name do
+ if find_heading("Domain Name")
+ @ast[:domain_name] = content_in_category
@weppos

weppos Nov 3, 2012

Owner

Please use strings as key instead of objects to allow GC. Also, in case you manually set a field, please use the form

@ast["field:domain_name"] = 'value'
+ tokenizer :get_registrant_details do
+ if find_heading("Registrant")
+ registrant_lines = content_in_category.split("\n")
+ @ast[:registrant_name] = registrant_lines.shift
@weppos

weppos Nov 3, 2012

Owner

Please keep the scanner simple. Delegate the job to map scanned values/positions to the parser.

This works better because most responses can return more or less detailed records for the same attribute (e.g. the Registrant attribute) and you don't really wont to deal with all these cases in a scanner.

See https://github.com/weppos/whois/blob/master/lib/whois/record/parser/whois.godaddy.com.rb#L89-128

Ideally, the scanner will simply returns an array of data and you'll map lines to attributes in the parser.

Owner

weppos commented Nov 11, 2012

According to weppos/whois-debian@43d3b75 there are a few more .za SLD changes.

Contributor

rorymckinley commented Nov 12, 2012

Yeah. As far as I know, the other .za SLDs are not as popular as .co.za. I am also done with all the changes you suggested (it's been a busy week) - and then I may take a look at those other SLDs

Contributor

rorymckinley commented Nov 12, 2012

I have made all the changes - would you like to see these as individual commits or would you prefer to have it rebased to a single commit?

Owner

weppos commented Nov 13, 2012

Hi @rorymckinley, you can merge all the commits into this branch and leave them separated. I normally prefer small, focused commits.

Thanks!

@ghost ghost assigned weppos Nov 13, 2012

Contributor

rorymckinley commented Nov 13, 2012

Ok - have pushed the fixes - they should now show up on this pull request as well. Thanks for the help and guidance thus far!

Contributor

rorymckinley commented Dec 1, 2012

Hi @weppos, just wondering if you have had a chance to look at the updates to this pull request yet?

Owner

weppos commented Dec 1, 2012

Actually, I haven't. I'll probably dedicate this weekend to review various
pull requests.

On Sat, Dec 1, 2012 at 7:27 PM, Rory McKinley notifications@github.comwrote:

Hi @weppos https://github.com/weppos, just wondering if you have had a
chance to look at the updates to this pull request yet?


Reply to this email directly or view it on GitHubhttps://github.com/weppos/whois/pull/191#issuecomment-10920243.

Simone Carletti
Passionate programmer and dive instructor

http://www.simonecarletti.com/
Twitter: @weppos https://twitter.com/weppos - Facebook: simone.io

Contributor

rorymckinley commented Dec 2, 2012

Awesome - thanks a lot!

@weppos weppos closed this in 1ba3858 Dec 9, 2012

weppos added a commit that referenced this pull request Dec 9, 2012

Owner

weppos commented Dec 9, 2012

Amazing patch @rorymckinley, a huge thank you for contributing to whois and sorry for keep you waiting so long.

I merged the pull request.

Contributor

rorymckinley commented Dec 9, 2012

Great - no problem for the wait - thank you for the help and patience!

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