Code review comment for lp:~vexo/bzr-webdav/bzr-webdav

Revision history for this message
Vincent Ladeuil (vila) wrote :

@Reagan: Sorry for the delay and thanks for working on that *and* adding tests.

I have almost nothing to comment, nice job ;)

I think my only question is: did you encounter issues with an existing DAV server (which one ?) or did you get there only by reading the RFC ?

Concretely, the only risk I can see with your patch is that by changing the client behavior we may now fail against servers which worked before... But IIUC, the only case where that could happen is if a server refuse to do a MOVE (with Overwrite: 'T') if the target does not exist. Thoughts ?

For the record:

39 +class QuirkyTestingDAVRequestHandler(TestingDAVRequestHandler):
40 + """
41 + A variant of TesttingDAVRequestHandler implementing various quirky/slightly
42 + off-spec behaviors to test how gracefully we handle them.
43 + """

PEP8 (or rather the variant we use in the bzr eco-system) asks for a single-line docstring so:
class QuirkyTestingDAVRequestHandler(TestingDAVRequestHandler):
    """Slightly off-spec behaviors.

    A variant of TesttingDAVRequestHandler implementing various quirky/slightly
    off-spec behaviors to test how gracefully we handle them.
    """

Or something, the idea is the first line should be a one-liner, more stuff could be added after a blank line.

78 - self._raise_curl_http_error(abspath, response,

Ouch !

106 - self._raise_curl_http_error(curl, 'unable to delete')

OUCH !

Damn, that's some lack of test coverage you fixed there :-/

Would you mind adding some to make sure we properly catch that kind of error ?

I guess that's how you discovered the bugs you filed though so 1) sorry
about that, 2) I'd really like to know which server you use ;)

If you can't address the above, just let know and I'll do it while landing, again, good work !

review: Needs Fixing

« Back to merge proposal