Port twisted.names.authority.BindAuthority to Py3 #579

hynek · 2016-10-31T12:38:39Z

Underway I had to fix a few bugs/annoyances. Most notably:

Remove a (debug!?) print statement when adding records.
Reflect explicit $ORIGIN in instance.
Add discouraging docstrings to public helper methods that should be private.
These should be deprecated ASAP.
Remove a bunch of commented out debug print statements.

I wrote a bunch of tests (the class was completely untested so far), but want to see Travis/coverage before writing more.

codecov-io · 2016-10-31T12:46:58Z

Current coverage is 90.74% (diff: 94.89%)

Merging #579 into trunk will increase coverage by 0.05%

@@              trunk       #579   diff @@
==========================================
  Files           815        815          
  Lines        145980     146021    +41   
  Methods           0          0          
  Messages          0          0          
  Branches      12875      12882     +7   
==========================================
+ Hits         132389     132505   +116   
+ Misses        11322      11245    -77   
- Partials       2269       2271     +2

Powered by Codecov. Last update c00f00e...d05819d

glyph · 2016-11-19T07:19:26Z

There are a couple of twistedchecker failures - can you fix those? Thanks!

Underway I had to fix a few bugs/annoyances. Most notably: - Fix a major bug in addRecord that didn't remove the final dot if an owner/origin is added. - Remove a (debug!?) print statement when adding records. - Reflect explicit $ORIGIN in instance. - Add discouraging docstrings to public helper methods that should be private. These should be deprecated ASAP. - Remove a bunch of commented out debug print statements.

hynek · 2016-11-19T10:02:45Z

Look at all those green build jobs!

adiroiban · 2016-11-19T11:26:08Z

Thanks for the patch. The release notes only talks about porting to python3.

Since in this branch you have also fixed a few bugs, shouldn't we advertise this in the release notes?

adiroiban · 2016-11-19T11:27:24Z

src/twisted/names/authority.py

-def getSerial(filename = '/tmp/twisted-names.serial'):
-    """Return a monotonically increasing (across program runs) integer.
+
+def getSerial(filename='/tmp/twisted-names.serial'):


since this is targeting python3, is filename a native string, bytes or unicode?

with the move to python3 I think that it would help to be explicit if something is text or data.

ISTM that for paths a filename is OK to be a native string. Are there any precedences?

I’d also like to point out, that I just touched this function to make twistedchecker shut up. Starting to fix it up seems out of scope? NB I’m not asking because I’m lazy, I’m asking because I’m not sure.

Fixing it is definitely out of scope. And we should probably figure out how to make twistedchecker not complain about stuff that is merely adjacent to your diff rather than part of it...

adiroiban · 2016-11-19T11:28:14Z

src/twisted/names/authority.py

-#            self._cache[(name, cls, type)] = r
-#            return r
-    serial = serial + ('%02d' % (ID,))
+        serialFile.write('%s %d' % (serial, id))


can we use FIlePath here? I think that it should take care of handling the path encoding issues.

adiroiban · 2016-11-19T11:31:46Z

src/twisted/names/authority.py

@@ -161,25 +156,32 @@ def _lookup(self, name, cls, type, timeout = None):
                else:
                    ttl = default_ttl

-                if record.TYPE == dns.NS and name.lower() != self.soa[0].lower():
+                if record.TYPE == dns.NS and \


I think that you should use parenthesis to break long lines .... but twistechecker should have complain about this

hynek · 2016-11-19T12:21:15Z

I thought about the release notes thing but wasn’t sure how to phrase it (especially because the bugs are rather subtle).

glyph

Thanks for doing this, @hynek . This is generally a great branch and I like most of the cleanups. However, if you could address the few nitpicks it would be great.

If you can push these changes to a branch on Twisted itself, then the all the buildbots should run and I think the coverage issues will be addressed. If not, please pacify codecov by adding tests for any uncovered lines.

glyph · 2016-11-22T20:55:44Z

src/twisted/names/authority.py

@@ -34,28 +44,14 @@ def getSerial(filename = '/tmp/twisted-names.serial'):
        os.umask(o)

    with open(filename, 'r') as serialFile:
-        lastSerial, ID = serialFile.readline().split()
+        lastSerial, id = serialFile.readline().split()


id is a builtin, which is why this was previously capitalized :).

Perhaps zoneID?

twistedchecker complained about ID

Technically (naming-standard-wise) it is correct to complain; it should just also be complaining about id. zoneID ought to be OK with it though.

glyph · 2016-11-22T20:56:19Z

src/twisted/names/authority.py

-    serial = serial + ('%02d' % (ID,))
+        serialFile.write('%s %d' % (serial, id))
+
+    serial = serial + ('%02d' % (id,))


Nitpick: .format instead of % because it's ~fancier~?

but it’s ~~slower~~? :)

glyph · 2016-11-22T20:57:51Z

src/twisted/names/authority.py

+        @type filename: L{bytes}
+        """
+        fp = FilePath(filename)
+        # XXX - not the best way to set an origin. It can be set using $ORIGIN


Per policy, XXX and FIXME comments should be links to tickets.

glyph · 2016-11-22T20:59:19Z

src/twisted/names/authority.py

-        return [
-            a.find(';') == -1 and a or a[:a.find(';')] for a in [
+        """
+        This is a private API; do not use in application code.


Can you make this method private and then add a deprecated public alias? Deprecation should be communicated structurally in a way that automated tooling can deal with, not with ad-hoc doc comments.

glyph · 2016-11-22T20:59:28Z

src/twisted/names/authority.py



    def collapseContinuations(self, lines):
-        L = []
+        """
+        This is a private API; do not use in application code.


Same as above comment.

glyph · 2016-11-22T21:00:12Z

src/twisted/names/authority.py

-        TTL = 60 * 60 * 3
-        ORIGIN = self.origin
+        """
+        This is a private API; do not use in application code.


See above about private APIs.

glyph · 2016-11-22T21:01:00Z

src/twisted/names/authority.py

-        if not domain.endswith('.'):
-            domain = domain + '.' + owner
+        """
+        This is a private API; do not use in application code.


OK hang on a second. Are these APIs really private? It feels like maybe somebody is using them. Maybe we can just delete the deprecations in this branch and do that separately?

How am I supposed to document them to make twistedchecker shut up?

That seems like an unrelated question, but... txchecker-travis is passing :).

You already have a docstring here, so if you just delete the line This is a private API; do not use in application code. it should still be fine with it.

Am I misunderstanding the question?

glyph · 2016-11-22T21:01:38Z

src/twisted/names/test/test_names.py

+
+    def test_aaaaRecords(self):
+        """
+        AAAA records are loaded correctly.


As per https://jml.io/pages/test-docstrings.html, "correctly" is a non-word :).

glyph · 2016-11-22T21:02:02Z

src/twisted/names/test/test_names.py

+
+    def test_mxRecords(self):
+        """
+        MX records are loaded correctly.


https://jml.io/pages/test-docstrings.html again

glyph · 2016-11-22T21:02:15Z

src/twisted/names/test/test_names.py

+
+    def test_invalidRecordClass(self):
+        """
+        Raises NotImplementedError on invalid records.


This sentence should have a subject.

glyph · 2016-11-23T19:21:55Z

src/twisted/names/authority.py

+        @param rdata: record data
+        @type rdata: L{list} of L{bytes}
+
+        @return: nothing


FYI: there's no return statement in the body of the function, so twistedchecker and pydoctor ought not to complain if you just omit @return entirely. And anywhere twistedchecker lets us omit stuff we probably should :). (If I'm wrong about this please correct me).

wsanchez · 2016-12-12T19:27:55Z

Coverage is failing.

At least one case is directly relevant to your changes: https://codecov.io/gh/twisted/twisted/compare/f520d35bfd552f0da91b248171d25789170110ae...1dd9c449504b07652af650382fc46b566fcd8546#7372632F747769737465642F6E616D65732F617574686F726974792E7079-86

Others are not new but triggering due to reformatting of code:
https://codecov.io/gh/twisted/twisted/compare/f520d35bfd552f0da91b248171d25789170110ae...1dd9c449504b07652af650382fc46b566fcd8546#7372632F747769737465642F6E616D65732F617574686F726974792E7079-86

Since recently policy says no overriding of these is allowed, you probably need to un-reformat that code or test it.

wsanchez · 2016-12-12T19:30:47Z

Actually I mis-read the "directly relevant" example; you removed an adjacent line, so again, cleanup is causing coverage to complain.

glyph · 2017-01-21T04:06:13Z

It looks like @hynek did in fact address all the review feedback, and the coverage issue is just a re-formatted NotImplementedError line. Formally speaking this is something that should be "in a different PR" but given that absolute coverage increased, I'm not going to be a pain in the butt about it :).

Since this has been a while I'm going to update the branch and push it to a twisted branch for full CI, but assuming CI is reasonably happy after that, I'll merge.

glyph · 2017-01-21T05:43:24Z

Thanks to https://www.traviscistatus.com/incidents/3pfp91tltsbr I really didn't want to wait the additional 2 hours for another trunk merge, so: merged.

glyph · 2017-01-21T05:44:23Z

(Happily the status just passed though, so no exception necessary!)

hynek added 2 commits November 19, 2016 10:16

Merge branch 'trunk' into 8880-bindauthority-py3

9b2dbe7

adiroiban reviewed Nov 19, 2016

View reviewed changes

Use parens for multi-line

87b9c2d

glyph reviewed Nov 22, 2016

View reviewed changes

glyph reviewed Nov 23, 2016

View reviewed changes

hynek added 2 commits December 10, 2016 12:52

Address review feedback

6965333

Merge branch 'trunk' into 8880-bindauthority-py3

1dd9c44

glyph added 2 commits January 5, 2017 15:38

Merge branch 'trunk' into 8880-bindauthority-py3

1d7f741

Merge branch 'trunk' into 8880-bindauthority-py3

d05819d

Merge branch 'trunk' into 8880-bindauthority-py3

0e0d7bf

glyph merged commit d34091c into twisted:trunk Jan 21, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port twisted.names.authority.BindAuthority to Py3 #579

Port twisted.names.authority.BindAuthority to Py3 #579

hynek commented Oct 31, 2016

codecov-io commented Oct 31, 2016 •

edited

Loading

glyph commented Nov 19, 2016

hynek commented Nov 19, 2016

adiroiban commented Nov 19, 2016

adiroiban Nov 19, 2016

hynek Nov 19, 2016

glyph Nov 23, 2016

adiroiban Nov 19, 2016

adiroiban Nov 19, 2016

hynek commented Nov 19, 2016

glyph left a comment

glyph Nov 22, 2016

hynek Nov 23, 2016

glyph Nov 23, 2016

glyph Nov 22, 2016

hynek Dec 10, 2016

glyph Nov 22, 2016

glyph Nov 22, 2016

glyph Nov 22, 2016

glyph Nov 22, 2016

glyph Nov 22, 2016

hynek Nov 23, 2016

glyph Nov 23, 2016

glyph Nov 22, 2016

glyph Nov 22, 2016

glyph Nov 22, 2016

glyph Nov 23, 2016

wsanchez commented Dec 12, 2016

wsanchez commented Dec 12, 2016

glyph commented Jan 21, 2017

glyph commented Jan 21, 2017

glyph commented Jan 21, 2017

Port twisted.names.authority.BindAuthority to Py3 #579

Port twisted.names.authority.BindAuthority to Py3 #579

Conversation

hynek commented Oct 31, 2016

codecov-io commented Oct 31, 2016 • edited Loading

Current coverage is 90.74% (diff: 94.89%)

glyph commented Nov 19, 2016

hynek commented Nov 19, 2016

adiroiban commented Nov 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hynek commented Nov 19, 2016

glyph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wsanchez commented Dec 12, 2016

wsanchez commented Dec 12, 2016

glyph commented Jan 21, 2017

glyph commented Jan 21, 2017

glyph commented Jan 21, 2017

codecov-io commented Oct 31, 2016 •

edited

Loading