[MERGE] add win32utils.get_local_appdata_location()

[Aaron Bentley]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Mark Hammond wrote:
>> ^^^ This seems like a missing feature, not a generic TestSkipped.
> 
> *shrug* - if I manually remove that environment variable from my environment
> then run the test, I'd be somewhat surprised if bzr asserted my operating
> system/bzr installation/whatever was suddenly missing a "feature"

We usually handle platform differences through features.  The feature
would be "Supplies APPDATA to shell commands" or something like that.

It is unfortunate that we cannot detect whether the OS supplies APPDATA,
only whether APPDATA is currently present.

> OTOH, having the test reported as being "skipped" *does*
> sound reasonable to me.

When we cannot run a test properly, we try to use one of KnownFailure,
UnavailableFeature or TestNotApplicable.  I try to avoid TestSkipped
because it is too generic.

>>> +    def test_local_appdata_matches_environment(self):
>>> +        # LOCALAPPDATA typically only exists on Vista, so we only
>> attempt to
>>> +        # compare when it exists.

>> ^^^ This shouldn't pass if there's no LOCALAPPDATA-- it's a missing
>> feature on pre-Vista Windows.
> 
> By "not pass", do you mean "fail"?  Or you just want a specific
> FeatureSkipped exception raised instead (but see above - its not clear to me
> what "feature" is missing other than an environment variable)?

I meant that it should raise UnavailableFeature, not that it should fail.

>>>  # CSIDL constants (from MSDN 2003)
>>>  CSIDL_APPDATA = 0x001A      # Application Data folder
>>> +CSIDL_LOCAL_APPDATA = 0x001c# <user name>\Local Settings\Applicaiton
>> Data (non roaming)
>>
>> ^^^^ "Application" is misspelled.
> 
> I believe you will find it mis-spelt in the MSDN header too.  Should I add
> "(sic)"?  Should I correct the spelling, remove the "from MSDN ..." comment?

I corrected the spelling before I submitted it.

>>> +    global has_win32com_shell
>>> +    if has_win32com_shell is None:
>>> +        try:
>>> +            from win32com.shell import shell
>>> +            has_win32com_shell = True
>>> +        except ImportError:
>>> +            has_win32com_shell = False
>> ^^^ This seems like it should be split out into a function.
> 
> Which part?  There are 7 new lines related to win32com.shell, but I wouldn't
> expect that to cross a threshold of when a new function should be created.
> No code is duplicated (indeed, the patch reduces duplication).  If this is a
> purely a personal style issue, please be very specific about what you want
> changed and I'd be happy to make it rather than discuss the finer points of
> that style.

This introduces a new public symbol, has_win32com_shell.  I assume that
it is public because it could have multiple clients.  I expect those
clients will want to know whether win32comm.shell is available, not
whether the test has been performed yet.

The API of has_win32com_shell is not very helpful to clients that want
to know whether win32comm.shell is available, because the value may be
None.  In order to handle that case, a client would need to implement
the same algorithm that _get_sh_special_folder_path already implements,
causing code duplication.

I think that it would be more useful if the public symbol were a
function that returns a boolean.  That would avoid future code
duplication and be easier for clients to use.

Or another possible API would return the module if it was present and
None if it was not.

Or we could make it not a public symbol.

So no, it has nothing to do with the length of that function.  In fact,
osutils has a whole bunch of very short "is x available" functions.

>>> +    if has_win32com_shell:
>>> +        # still need to bind the name locally, but this is fast.
>>> +        from win32com.shell import shell
>> ^^^ if has_win32com_shell is True, won't shell already be imported?
> 
> I believe that as the comment says, 'shell' will be an unbound local without
> that block every time the function is called after the first.  Is the
> comment unclear?  If so, how should I adjust it?

I was considering only the case where has_win32com_shell starts out as
None, and then is determined to be True.  In that case, "shell" will
already be assigned when we enter the if block.  But as you say, it
would be an unbound local every time after that.  So never mind.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFI2cac0F+nu1YWqI0RApr0AJ40DmqCvrROI0OkCiEG87kpGN1JGwCfU+B4
57xlIuUKABBSLTiLVkE4B8A=
=Ft9t
-----END PGP SIGNATURE-----