Updated patch attached based on code reviews.

I'm replying to the code review here as when I tried to reply on bugs.python.org/review I got a Python exception, "AttributeError: NoneType has no attribute something or other". FYI, it seems Django is set up in debug mode, because it was the full colourful traceback with local vars etc. I didn't save this file, sorry!

Zach Ware
=========

> Either way, these should probably be defined in stat.py as well (sorry; I should
> have said 'as well as' instead of 'instead of' when I mentioned _stat.c in the
> email thread).

I don't think so -- I thought the purpose of moving them to _stat.c was so you could use the windows.h constants rather than hard-coded integers? But if they're in stat.py, why do they need to be in _stat.c as well?

http://bugs.python.org/review/21719/diff/12149/Modules/_stat.c#newcode563
> Modules/_stat.c:563: // For some reason FILE_ATTRIBUTE_INTEGRITY_STREAM and
> PEP 7 bans C++ style comments.

Good point.


stoneleaf
=========

> 'covert' should be 'convert' - can you fix this as part of your patch? ;)

Fixed in latest patch.

> Instead of spelling out FILE_ATTRIBUTE_ each time, can we abbreviate that part
> to FA_ ?

I disagree. I realize FILE_ATTRIBUTE_* is longish, but it's what it's called in the Windows API, so will make these constants much more findable and obvious to Windows programmers (consistent with the Win32 documentation).

> On non-Windows machines 'st_file_attributes' will be 0?

No, it won't be present at all (like the other non-cross-platform attributes).

> There's no attribute for the value 8?

Correct -- see the Win32 API documentation and loewis's comment.



haypo
=====

> It would be nice to document these options here :)

I don't think we should duplicate the Win32 documentation. Ours will just be less thorough and potentially get out of date. I agree we should add a link to the Win32 docs though.

> Instead of == 0, you can use assertFalse.

Could, but I don't like this when I'm doing bit/integer comparisons. Explicit is better.

> The purpose of the _stat module is to only provide data from the OS, to not
> hardcode constants. So -1 to provide these constants on all platforms.

Agreed. See above.

> If the constant is only available on new Visual Studio version, we may hardcode
> the constant to avoid the dependency on a specific Visual Studio version. For
> example, in unicodeobject.c I used:
>
> #ifndef WC_ERR_INVALID_CHARS
> #  define WC_ERR_INVALID_CHARS 0x0080
> #endif

That's a good way of doing it. I'll use this approach.

> Modules/posixmodule.c:1437: unsigned long st_file_attributes;
> Should we use a DWORD here?

I don't think so. None of the other values in win32_stat (e.g., st_dev) are defined using DWORD or Win32-style types. See also loewis's comment.


loewis
======

> We shouldn't replicate Microsoft's documentation (if for no other reason than
> copyright). Instead, linking to
> 
> http://msdn.microsoft.com/en-us/library/windows/desktop/gg258117.aspx
>
> might be helpful (as long as Microsoft preserves this URL).

Agreed. I'll make this change.