Code review comment for lp:~jelmer/wikkid/dulwich

Revision history for this message
Gavin Panella (allenap) wrote :

I'm not sure that I'm really qualified to review this, but cosmic
radiation seems to have flipped a bit in Launchpad's database such that
I have been invited to pass judgement.

[1]

+            try:
+                (mode, sha) = tree[el]
+                if not stat.S_ISDIR(mode):
+                    raise FileExists(
+                        "File %s exists and is not a directory" % el)
+            except KeyError:
+                tree = Tree()
+            else:
+                tree = self.store[sha]

I'm guessing that you're not trying to guard against a KeyError in the
`if not stat.S_ISDIR(...` block, so it probably ought to go in the else:
block.

[2]

+                ret.append(
+                    File(self.store, mode, sha, posixpath.join(directory_path, name), commit_id))

In places you use "/" as the separator. Consider using posixpath.sep
for, I don't know, cleanliness?

[3]

Can you add dulwich as a dependency in setup.py?

[4]

There are a lot of new things here without obvious tests, like the
middleware, but given that there's little other development activity I
hardly want to block for that. In other words, you're very naughty for
not testing things, but there's no punishment, and thanks for the
contribution :)

review: Approve

« Back to merge proposal