Opened 5 years ago

Last modified 4 years ago

#6086 enhancement new

Finish implementing twisted.names.dns.str2time

Reported by: Jean-Paul Calderone Owned by: Nic Young
Priority: normal Milestone:
Component: names Keywords:
Cc: Nic Young, Richard Wall Branch:
Author:

Description (last modified by Jean-Paul Calderone)

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 Nic Young 5 years ago.
uses re module to parse string to create tuples for summing the interval value
patch6086.2.patch (2.8 KB) - added by Nic Young 5 years ago.
patch6068.3.patch (3.4 KB) - added by Nic Young 5 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 5 years ago by Jean-Paul Calderone

Description: modified (diff)
Keywords: easy added

Changed 5 years ago by Nic Young

Attachment: patch6086.patch added

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

comment:2 Changed 5 years ago by Nic Young

Cc: Nic Young added
Keywords: review added
Owner: set to Nic Young

comment:3 Changed 5 years ago by Jean-Paul Calderone

Owner: Nic Young deleted

assign tickets for review to noone

comment:4 Changed 5 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Nic Young

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 5 years ago by Nic Young

Attachment: patch6086.2.patch added

comment:5 in reply to:  4 Changed 5 years ago by Nic Young

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 5 years ago by Nic Young

Owner: Nic Young deleted

comment:7 Changed 5 years ago by therve

Keywords: review removed
Owner: set to Nic Young

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 5 years ago by Nic Young

Attachment: patch6068.3.patch added

comment:8 Changed 5 years ago by Nic Young

Keywords: review added
Owner: Nic Young 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 5 years ago by Richard Wall

Owner: set to Richard Wall
Status: newassigned

Reviewing latest patch...

comment:10 Changed 5 years ago by Richard Wall

Cc: Richard Wall added
Keywords: review removed
Owner: changed from Richard Wall to Nic Young
Status: assignednew

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 Changed 5 years ago by Jean-Paul Calderone

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 5 years ago by Richard Wall

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 4 years ago by Itamar Turner-Trauring

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