Finish implementing twisted.names.dns.str2time

[exarkun]

str2time should be able to parse strings like "2W3D4H" representing the interval "two weeks, three days, four hours". Presently it can only parse strings like "2W" or "3D" or "4H". In other words, it cannot parse strings containing multiple interval specifiers. It can only parse strings made up of a single interval specifier.

[nry]

uses re module to parse string to create tuples for summing the interval value

[exarkun]

assign tickets for review to noone

[exarkun]

Thanks.

1. This feature needs unit tests.
2. The docstring for str2time also needs to be updated to explain the multiple time unit support being added.
3. interval_sum needs to be renamed to comply with the naming convention
4. This implementation will accept things like "123Z" which is meaningless and should be rejected with an error. In general, make sure to reject any invalid inputs rather than ignore them silently.

Thanks again!

[nry]

Replying to exarkun:

Thanks. 1. This feature needs unit tests. 1. The docstring for str2time also needs to be updated to explain the multiple time unit support being added. 1. interval_sum needs to be renamed to comply with the naming convention 1. This implementation will accept things like "123Z" which is meaningless and should be rejected with an error. In general, make sure to reject any invalid inputs rather than ignore them silently. Thanks again!

I Made all the changes you suggested. Please let me know if any thing else needs to be changed.

Thanks for the feedback!

[therve]

Sorry for not reviewing earlier. Some comments:

• You're looping over suffixes and then interval, that's quite inefficient. Maybe turning suffixes into a dict would prevent one loop.

• The current implementation still ignores lots if invalid input instead of raising an error. For example AA6Y or 6YAA is parsed the same as 6Y

• The last part of the function (the cast to int) is now unreachable and should be removed.

Thanks!

[nry]

Replying to therve:

Sorry for not reviewing earlier. Some comments: * You're looping over suffixes and then interval, that's quite inefficient. Maybe turning suffixes into a dict would prevent one loop. * The current implementation still ignores lots if invalid input instead of raising an error. For example AA6Y or 6YAA is parsed the same as 6Y * The last part of the function (the cast to int) is now unreachable and should be removed. Thanks!

Hi there,

* I have turned both the suffixes tuple and the interval tuple in to dictionaries and now it only loops once.

* Changed the implementation to detect invalid input.

* Removed the last part of the function.

[rwall]

Reviewing latest patch...

[rwall]

Code Review

Thanks nry, the latest patch looks much better but I think some more work is needed.

Here are some comments:

1. Add a news file (see wiki:ReviewProcess#Newsfiles)
2. Remove all trailing white space. ( http://twistedmatrix.com/documents/current/core/development/policy/coding-standard.html#auto5)
3. "str2time"
   1. I can't find an RFC for the Bind zone ttl formats but it's worth examining the source from other projects...
      1.  https://bazaar.launchpad.net/~ubuntu-branches/ubuntu/raring/bind9/raring/view/head:/lib/dns/ttl.c#L151
      2.  http://wiki.powerdns.com/trac/browser/trunk/pdns/pdns/zoneparser-tng.cc#L79
      3.  https://bazaar.launchpad.net/~ubuntu-branches/ubuntu/raring/nsd/raring/view/head:/util.c#L291
   2. ...and some unofficial ttl documention...
      1.  http://www.zytrax.com/books/dns/apa/time.html
   3. Neither of those mentions Y (year) interval units - so I'd remove the parsing of Y.
   4. Both suggest that the interval unit should be treated case insensitively.
   5. Your regexp '[^{\d|S|M|H|D|W|Y]' looks wrong - there's no need to separate each character in a bracket expression with |.}
   6. If you're going to compile the regexp you may as well do it once and store it as a private module variable.
   7. I'd be inclined to use re.split then loop through the resulting parts checking strictly for alternating positive integers, then optional (recognised) interval unit character. eg

             >>> r = re.compile(r'([WDMHS])', flags=re.IGNORECASE)
             >>> r.split('1w2d3h4m5s')
             ['1', 'w', '2', 'd', '3', 'h', '4', 'm', '5', 's', '']
      

   8. Or alternatively don't use regular expressions at all and just parse the string one char at a time.
   9. Maybe check that the interval units occur in descending order (eg not DSMW) and that no interval suffix occurs more than once. (eg 1W2D2D) (although Bind doesn't seem to be that strict)
   10. The Bind source says "No legal counter / ttl is longer that 63

characters." maybe you should enforce that too.

4. "test_dns.py"
   1. I don't think a single test_combo test is sufficient
   2. I think you should modify all the existing tests "test_hour" "test_day" etc to test both the individual and the combo case and with both upper and lower case. eg 1d 1D 0W1D0H0M0S.
   3. Add a new test to test that invalid an suffix raises a ValueError - test with both individual and combo assertions eg 1X and 1x2y3z
   4. Maybe Test that repeated suffixes raise ValueError
   5. Maybe Test that unordered suffixes raise ValueError

Sorry for the lengthy review and thanks again for your efforts so far.

It'll be nice to see the Twisted DNS server get "better than Bind" Bind zone parsing capabilities!

-RichardW.

[exarkun]

Neither of those mentions Y (year) interval units - so I'd remove the parsing of Y

Despite the lack of documentation or support elsewhere for this, we should not just remove it. On upgrading to a version of Twisted which had removed supported this, any deployment which used the feature would immediately break. It doesn't matter if BIND doesn't support this - that only would have been a good reason not to implement it in the first place. Since it was implemented (and is even tested!), it's a feature. If we want to remove it, we need to follow the deprecation process outlined on CompatibilityPolicy. My take on this is that having it doesn't really do much harm, and removing it is more effort than it is worth.

[rwall]

Replying to exarkun:

If we want to remove it, we need to follow the deprecation process outlined on CompatibilityPolicy.

Ah I hadn't thought of that. Point taken.

My take on this is that having it doesn't really do much harm, and removing it is more effort than it is worth.

Yes I agree.