#967 closed enhancement (fixed)

minor code clean-up in dirnode.py

Reported by: zooko Owned by:
Priority: minor Milestone: 1.7.1
Component: code-dirnodes Version: 1.6.0
Keywords: easy reviewed Cc:
Launchpad Bug:

Description

I don't want to commit this to trunk until after v1.6.1 release, but please go ahead and review it.

Impose micro-POLA by passing only the writekey instead of the whole node object to _encrypt_rw_uri(). Remove DummyImmutableFileNode? in nodemaker.py, which is obviated by this. Add micro-optimization by precomputing the netstring of the empty string and branching on whether the writekey is present or not outside of _encrypt_rw_uri(). Add doc about writekey to docstring.

Attachments (2)

dirnode-minor-cleanup.darcs.patch.txt (8.6 KB) - added by zooko at 2010-02-21T05:35:55Z.
dirnodecleanup.dpatch.txt (11.5 KB) - added by zooko at 2010-07-11T04:44:21Z.

Download all attachments as: .zip

Change History (13)

comment:1 Changed at 2010-04-04T01:59:35Z by kevan

All of the tests pass when the patch is applied.

The changes to _encrypt_rw_uri look good, and you don't appear to have missed any of its callers. I think that the same can be said for pack_children; I don't see any problems with those changes, and you appear to have changed all of its callers that needed changing.

There appears to be trailing whitespace on line 188 of dirnode.py (right after assert isinstance(rw_uri, str), rw_uri). Your patch didn't introduce that, but, as long as you're doing code cleanup, you may as well fix it. :)

Unless I'm missing something, DummyImmutableFileNode? is no longer necessary. If you agree, then you should remove it from nodemaker.py.

Other than that, this looks good.

comment:2 Changed at 2010-04-04T02:06:00Z by kevan

  • Keywords review-needed removed

comment:3 Changed at 2010-06-17T05:00:35Z by zooko

Heh, then I forgot to commit it and now I don't want to commit it before the v1.7.0 release.

However! I have created a "post-1.7" branch, and I can commit it to that...

20100221052527-92b7f-3ee334f0e78c3890d3f8b8d2b5d9b440b2a5343d/post-1.7/

comment:4 Changed at 2010-06-17T05:06:45Z by zooko

  • Milestone changed from 1.7.0 to soon
  • Resolution set to fixed
  • Status changed from new to closed

Applied your two clean-up suggestions in 20100617045339-92b7f-940b4924686845e26ec03bd6c7d6094beec14c7e/post-1.7. Thanks!

Last edited at 2010-06-17T05:07:23Z by zooko (previous) (diff)

comment:5 Changed at 2010-06-17T23:44:33Z by zooko

<warner> zooko: your tidy-ups in the post-1.7 branch look good. You might
	 consider doing "packed = pack_children(children, writekey=None)" in
	 nodemaker.py to make it more obvious that it's creating a
	 writecap-less structure				        [17:20]
<warner> in fact, I'd consider making the writekey= argument mandatory, since
	 I think the only place that ever calls it with a known-None value is
	 that line in nodemaker.py				        [17:21]

Changed at 2010-07-11T04:44:21Z by zooko

comment:6 Changed at 2010-07-11T04:44:31Z by zooko

  • Keywords review-needed added
  • Milestone changed from soon to 1.7.1
  • Resolution fixed deleted
  • Status changed from closed to reopened

reopen to port to trunk (from the post-1.7 branch) and apply Brian's suggestion from comment:5. Please review!

comment:7 Changed at 2010-07-17T07:24:51Z by zooko

  • Milestone changed from 1.7.1 to 1.8β

Not a bugfix, so not going into 1.7.1.

comment:8 Changed at 2010-07-17T23:44:30Z by davidsarah

  • Milestone changed from 1.8β to 1.7.1
  • Resolution set to fixed
  • Status changed from reopened to closed

Applied in 6e8477114e9dfd06. Was that intentional?

comment:9 Changed at 2010-07-17T23:47:26Z by davidsarah

  • Keywords reviewed added; review-needed removed

FWIW, the port to trunk looks correct.

comment:10 Changed at 2010-07-18T00:08:23Z by zooko

That was not intentional. I accidentally committed this patch to trunk. Haven't decided yet whether to undo it for 1.7.1 or just leave it.

comment:11 Changed at 2010-07-18T03:57:50Z by davidsarah

I checked that your port is equivalent to the similar version that was on the post1.7 branch. (The only difference was whether the writekey argument has a default -- in your version it doesn't, which is better.)

Note: See TracTickets for help on using tickets.