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

Revision history for this message
Natalia Bidart (nataliabidart) wrote :

* Can you please replace:
        for char in WINDOWS_ILLEGAL_CHARS_MAP:
            self.testfile = self.testfile + char
with:
    self.testfile = ''.join(WINDOWS_ILLEGAL_CHARS_MAP)

* typos: "illechal", "illegla"

* can you please use os.path.splitdrive(path) (http://docs.python.org/library/os.path.html#os.path.splitdrive) instead of the custom logic in diff line 392 (and everywhere else where you split the volume letter)?

* can you replace:

    for illegal_char in WINDOWS_ILLEGAL_CHARS_MAP:
        if illegal_char in path:
            return True

with:

    any(c in WINDOWS_ILLEGAL_CHARS_MAP for c in path)

* can you please replace:

        for illegal_char in WINDOWS_ILLEGAL_CHARS_MAP:
            partial = partial.replace(illegal_char,
                                      WINDOWS_ILLEGAL_CHARS_MAP[illegal_char])

with:

       for illegal_char, replacement in WINDOWS_ILLEGAL_CHARS_MAP.iteritems():
            partial = partial.replace(illegal_char, replacement)

* can you please add a logger.exception statement inside the except OSError, e: within set_no_rights? it will be useful to diagnose potential issues from log reports from our users. Same for remove_file, remove_dir, make_dir, open_file, rename, stat_path.

* why are you doing this in abspath? path = path.replace('/', '\\') Seems unnecessary and may hide bugs in our code regarding multiplatform path management.

review: Needs Fixing

« Back to merge proposal