Code review comment for lp:~mnn282/bzr/sftp-unsupported-operation-more-info

Martin Packman (gz) wrote :

Thanks for making those changes, not all the suggestions I made were completely correct so I hope you don't mind poking some of the minor details again.

Style nit, two lines between top level things in modules, see:
<http://www.python.org/dev/peps/pep-0008/#blank-lines>

+ self.message = "\n" + message

So, I said 'msg' wasn't a great name, but annoyingly 'message' is worse due to some legacy Python exception stuff. How about 'description' or something?

Also, having english text as a param prevents that part from being translated, but this isn't too important.

Including the path, and just using the operation name rather than the text form would work for everything but the symlink/rename case, which could just be an optional target_path param to the exception.

+ self.assertEquals(e.message, "\nunable to symlink")
\ No newline at end of file

As with the other mp, this needs a terminal newline or bt.test_source fails.

+ @staticmethod
+ def _translate_io_exception(e, path, more_info='',
...
- self._translate_error(e, path, raise_generic=False)
+ Transport._translate_error(e, path, raise_generic=False)

The neat way around needing the Transport import for this is to instead do:

    @classmethod
    def _translate_io_exception(cls, e, path, more_info='',
...
        cls._translate_error(e, path, raise_generic=False)

Leaving _translate_error as @staticerror is fine.

+ # raise errors.TransportNotPossible(more_info)

Just delete this.

review: Needs Fixing

« Back to merge proposal