Ticket #6086 enhancement new

Opened 18 months ago

Last modified 12 months ago

Finish implementing twisted.names.dns.str2time

Reported by: exarkun Owned by: nry
Priority: normal Milestone:
Component: names Keywords:
Cc: nryoung@…, richard@… Branch:
Author: Launchpad Bug:

Description (last modified by exarkun) (diff)

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.

Attachments

patch6086.patch Download (0.9 KB) - added by nry 18 months ago.
uses re module to parse string to create tuples for summing the interval value
patch6086.2.patch Download (2.8 KB) - added by nry 17 months ago.
patch6068.3.patch Download (3.4 KB) - added by nry 15 months ago.

Change History

1

  Changed 18 months ago by exarkun

  • keywords easy added
  • description modified (diff)

Changed 18 months ago by nry

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

2

  Changed 18 months ago by nry

  • owner set to nry
  • keywords review added
  • cc nryoung@… added

3

  Changed 18 months ago by exarkun

  • owner nry deleted

assign tickets for review to noone

4

follow-up: ↓ 5   Changed 18 months ago by exarkun

  • owner set to nry
  • keywords review removed

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!

Changed 17 months ago by nry

5

in reply to: ↑ 4   Changed 17 months ago by nry

  • keywords review added

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!

6

  Changed 17 months ago by nry

  • owner nry deleted

7

  Changed 16 months ago by therve

  • owner set to nry
  • keywords review removed

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!

Changed 15 months ago by nry

8

  Changed 15 months ago by nry

  • keywords review added
  • owner nry deleted

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.

9

  Changed 15 months ago by rwall

  • owner set to rwall
  • status changed from new to assigned

Reviewing latest patch...

10

  Changed 15 months ago by rwall

  • owner changed from rwall to nry
  • keywords review removed
  • status changed from assigned to new
  • cc richard@… added

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.

  1. "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.

11

follow-up: ↓ 12   Changed 15 months ago by 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.

12

in reply to: ↑ 11   Changed 15 months ago by 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.

13

  Changed 12 months ago by itamar

  • keywords easy removed
Note: See TracTickets for help on using tickets.