Code review comment for lp:~mandel/ubuntuone-client/illegal_windows_chars

Revision history for this message
Alejandro J. Cura (alecu) wrote :

"if e.winerror and e.winerror == 123" should be formulated as """if getattr(e, "winerror", None) == 123""", in case the exception has no winerror attribute. And it should perhaps be moved into a function called let's say "checkWinError123(e)", since it appears at least 8 times through the branch.

calling set_no_rights from inside set_no_rights (in the case of errors) risks the possibility of entering an infinite recursion loop; I suggest moving the three lines inside the try to a small new function, and calling it twice, once from the try, and once from the "if e.winerror...".

typos:
 * "illechal" -> "illegal"
 * "there reason" -> "the reason"
 * spurious spaces have appeared at the end of lines 487 and 496 of the diff
 * "illegla" -> "illegal"

review: Needs Fixing

« Back to merge proposal