[MERGE] Add some unicode-related tests from the hpss branch, and a few other nits (also from the hpss branch).
andrew at canonical.com
Mon Apr 16 17:39:28 BST 2007
John Arbash Meinel wrote:
> >> will fail for anything but args == tuple.
> > I think this is a YAGNI. It's easy to make more flexible later if we decide
> > it's worth it, but so far the hpss branch hasn't had trouble with using tuples.
> > Perhaps because the current protocol is largely about encoding and decoding
> > tuples? Anyway, I'm not worried about this.
> ^- I think it is a YAGNI, but I also think it is something that non-uber
> hackers could stumble on.... Would they stumble very hard?
> >>> x = (1,2,3)
> >>> x + [4, 5]
> Traceback (most recent call last):
> File "<stdin>", line 1, in <module>
> TypeError: can only concatenate tuple (not "list") to tuple
> Is probably sufficient for them to know what to do.
> > Although it seems almost too obvious to be worth stating, I've expanded the
> > docstring like so:
> > def remote_path_from_transport(self, transport):
> > """Convert transport into a path suitable for using in a request.
> > Note that the resulting remote path doesn't encode the host name or
> > anything but path, so it is only safe to use it in requests sent over
> > the medium from the matching transport.
> > """
> ^- Well, too obvious to you, and understandable to me :) This is a
> really nice docstring. Thank you. I honestly didn't correctly understand
> the function before this.
I'm glad it helped :)
It's tricky writing a clear docstring for something that seems obvious to the
person writing it!
> > I'd like there to be direct tests, but at this point it's not a big concern for
> > me; I'm confident in this code anyway, and SmartClient doesn't change very much
> > so the lack of more focussed tests isn't much of a burden to developing it. If
> > SmartClient evolves more later, that would be a good time to define tests for it
> > I think.
> ^- I really don't like "public" code that isn't tested. Once we have a
> release, we have a backwards compatibility statement. (Which we
> certainly seem to break regularly, but we try not to). Without failing
> tests, you don't even know if you've broken compatibility.
> If SmartClient is meant as an implementation detail, then it should be
> marked private IMO.
> You seem to be writing tests to make sure the code you've written is
> correct (which is valid). I also write them to make sure other people
> updating the code don't mess it up.
That's a fair point. I think the pragmatic thing to do for now is to rename
SmartClient to _SmartClient for 0.16, which clearly makes it an implementation
detail and not a public feature.
> > Yep, it passes. Worth checking though! :)
(And it turns out it failed on 2.4 but not 2.5, now fixed).
> > Any chance of an upgrade to +1?
> > -Andrew.
> I don't want to get bogged down without getting the real improvements
> merged. I think there are some testing philosophy discussions to be had,
> but the changes are fine. (+1).
More information about the bazaar