Code review comment for lp:~luoyonggang/subvertpy/unittest-win32

Am 12/03/12 02:10, schrieb Yonggang Luo:
> 2012/3/12 Yonggang Luo<email address hidden>
>
>> 2012/3/12 Jelmer Vernooij<email address hidden>
>>
>>>> 2012/3/11 Jelmer Vernooij<email address hidden>
>>>>
>>>>>> Please read the test result carefully. And you will know why do the
>>>>> things in
>>>>>> this way.
>>>>>> The test result is under
>>>>>>
>>>>>> Description of the Change
>>>>> That only explains *what* is failing for you. It doesn't explain
>> *why*
>>> it
>>>>> is failing, and why your fix addresses that. That's the bit I'm
>>> interested
>>>>> in.
>>>> It's failed just because Python's maketemp didn't return the corrent
>>>> tempfile file path.
>>>> For example, tempfile.mkdtemp() on windows will gives us with the file
>>> path
>>>> c:\users\uername\appdata\local\temp\hash_code , but the real file path
>>>> is C:\Users\username\AppData\Local\Temp\hash_code.
>>>> When I use
>>>> tempfile.mkdtemp(dir =
>>>> os.getenv("TEMP", None) or os.getenv("TMP", None))
>>>> it's will give us the exact file path with the windows file system.
>>>> TEMP and TMP is recorded by the OS. and even if TEMP and TMP is not
>>> exist,
>>>> it's sill works, just because dir accept None as
>>>> a default input.
>>> TEMP and TMP can be set by the user too as far as I know. What if the
>> user
>>> has set them to "C:\users\USeRName\AppData\Local\Temp\hash_code" ?
>> That's a
>>> perfectly valid path, as path lookups are case insensitive.
>>>
>>> If TEMP and TMP do not exist, we get the same behaviour as currently so
>>> users who have that will still hit that bg - and that means this fix is
>>> incomplete in that situation.
>>>
>>>> The unittest failed, is just because Subversion changed the file path
>>>> from c:\users\uername\appdata\local\temp\hash_code
>>>> to C:\users\uername\appdata\local\temp\hash_code, but under Windows
>> file
>>>> system, these two path represents the same path,
>>>> so it's should not be FAILED.
>>> Then why not simply call os.path.normcase() on both paths before
>> comparing
>>> them? That seems like a much cleaner solution to me.
>>>
>> That's why I ask you better solution, thanks.
>>
> It's not works, still return the path with incorrect case.
Can you please elaborate? What doesn't work ? Does os.path.normcase not
return a normalized case?

Cheers,

Jelmer