Opened 2 years ago

Last modified 19 months ago

#6086 enhancement new

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)

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 (3)

patch6086.patch (919 bytes) - added by nry 2 years ago.
uses re module to parse string to create tuples for summing the interval value
patch6086.2.patch (2.8 KB) - added by nry 2 years ago.
patch6068.3.patch (3.4 KB) - added by nry 22 months ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 2 years ago by exarkun

  • Description modified (diff)
  • Keywords easy added

Changed 2 years ago by nry

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

comment:2 Changed 2 years ago by nry

  • Cc nryoung@… added
  • Keywords review added
  • Owner set to nry

comment:3 Changed 2 years ago by exarkun

  • Owner nry deleted

assign tickets for review to noone

comment:4 follow-up: Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner set to nry

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 2 years ago by nry

comment:5 in reply to: ↑ 4 Changed 2 years ago by nry

  • Keywords review added

Replying to 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!

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

Thanks for the feedback!

comment:6 Changed 23 months ago by nry

  • Owner nry deleted

comment:7 Changed 22 months ago by therve

  • Keywords review removed
  • Owner set to nry

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 22 months ago by nry

comment:8 Changed 22 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.

comment:9 Changed 21 months ago by rwall

  • Owner set to rwall
  • Status changed from new to assigned

Reviewing latest patch...

comment:10 Changed 21 months ago by rwall

  • Cc richard@… added
  • Keywords review removed
  • Owner changed from rwall to nry
  • Status changed from assigned to new

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.

comment:11 follow-up: Changed 21 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.

comment:12 in reply to: ↑ 11 Changed 21 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.

comment:13 Changed 19 months ago by itamar

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