Merge lp:~mnn282/bzr/sftp-unsupported-operation-more-info into lp:bzr
Status: | Needs review |
---|---|
Proposed branch: | lp:~mnn282/bzr/sftp-unsupported-operation-more-info |
Merge into: | lp:bzr |
Diff against target: |
218 lines (+70/-15) 4 files modified
bzrlib/errors.py (+10/-0) bzrlib/tests/test_sftp_transport.py (+37/-0) bzrlib/transport/__init__.py (+5/-4) bzrlib/transport/sftp.py (+18/-11) |
To merge this branch: | bzr merge lp:~mnn282/bzr/sftp-unsupported-operation-more-info |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Martin Packman (community) | 2012-07-26 | Needs Fixing on 2012-07-30 | |
Review via email:
|
Description of the change
Unsupported operations now have more information in SFTP transport.
mnn (mnn282) wrote : | # |
Martin Packman: So, the handling has been moved into _translate_
I was told on IRC that I should make _translate_
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://
+ 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.assertEqua
\ No newline at end of file
As with the other mp, this needs a terminal newline or bt.test_source fails.
+ @staticmethod
+ def _translate_
...
- self._translate
+ Transport.
The neat way around needing the Transport import for this is to instead do:
@classmethod
def _translate_
...
Leaving _translate_error as @staticerror is fine.
+ # raise errors.
Just delete this.
mnn (mnn282) wrote : | # |
> Also, having english text as a param prevents that part from being translated, but this isn't too important.
But other exception handling code in SFTPTransport has only english text as parameter too.
> 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.
This would require altering _translate_
Martin Packman (gz) wrote : | # |
> But other exception handling code in SFTPTransport has only english text as
> parameter too.
Right I wouldn't worry to much about doing this.
> This would require altering _translate_
I think passing the path to the exception is potentially useful, and shouldn't mean too much extra fiddling.
Unmerged revisions
- 6547. By mnn on 2012-07-31
-
- SFTPTransport.
_translate_ io_exception changed to @classmethod
- changed TransportOperationNotSupported .message to description - 6546. By mnn on 2012-07-27
-
Moved handling of unsupported operations into SFTPTransport.
_translate_ io_exception, which has now optional argument 'operation' SFTPTransport.
_translate_ io_exception and Transport. _translate_ error are now static methods - 6545. By mnn on 2012-07-26
-
Display more information for TransportOperat
ionNotSupported (for rename - path from and path to) - 6544. By mnn on 2012-07-25
-
Unsupported operations now have more information in SFTP transport
- stubs for unsupported transport operations (TransportOpera
tionNotSupporte d) - used by rename operations in SFTP transport
Idea of adding a new exception here seems reasonable.
You'll want to add it inside _translate_ io_exception rather than adding a special separate branch before hand. That might mean tweaking that method a little, but it's private so there's no reason to to adapt it.
You want some tests as well, you can probably just test the translation itself rather than trying to fake out paramiko to raise the exception you're observing.