Ben Hoyt added the comment: 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. ---------- Added file: http://bugs.python.org/file35621/issue21719-2.patch _______________________________________ Python tracker <rep...@bugs.python.org> <http://bugs.python.org/issue21719> _______________________________________ _______________________________________________ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
