[issue5915] PEP 383 implementation

["Martin v. Löwis" <report@bugs.python.org> at psf.upfronthosting.co.za]

Martin v. Löwis <martin at v.loewis.de> added the comment:

Reviewers: report_bugs.python.org, Benjamin,

Message:
Fixed in r72272

http://codereview.appspot.com/52095/diff/1/5
File Include/unicodeobject.h (right):

http://codereview.appspot.com/52095/diff/1/5#newcode1254
Line 1254: The function is intended to be used for paths and file names
only
On 2009/05/03 21:34:26, Benjamin wrote:
> If these are only meant to be used internally during bootstrap, should
they have
> the _Py prefix?

Perhaps. However, I only moved the declarations from a different part of
the file, so I feel renaming them is out of scope for this patch. Only
PyUnicode_FSConverter is new.

http://codereview.appspot.com/52095/diff/1/10
File Lib/test/test_pep383.py (right):

http://codereview.appspot.com/52095/diff/1/10#newcode1
Line 1: from test import support
On 2009/05/03 21:34:26, Benjamin wrote:
> I'm not sure this deserves a whole new file. Couldn't UtfbTest go in
test_codecs
> and FileTests go in test_os?

Done.

http://codereview.appspot.com/52095/diff/1/10#newcode2
Line 2: import unittest, shutil, os, sys
On 2009/05/03 21:34:26, Benjamin wrote:
> PEP 8 says these should be on separate lines.

Done.

http://codereview.appspot.com/52095/diff/1/10#newcode33
Line 33: if os.name != 'win32':
On 2009/05/03 21:34:26, Benjamin wrote:
> Isn't os.name "nt" on Windows?

class with
> @unittest.skipIf(sys.platform.startswith("win")).

Done. It's now in a non-Win32 section of test_os.

http://codereview.appspot.com/52095/diff/1/10#newcode47
Line 47: f = open(self.bdir + b"/" + fn, "w")
On 2009/05/03 21:34:26, Benjamin wrote:
> Looks like a good place for os.path.join.

Done.

http://codereview.appspot.com/52095/diff/1/13
File Modules/posixmodule.c (right):

http://codereview.appspot.com/52095/diff/1/13#newcode547
Line 547: else if(PyByteArray_Check(o)) {
On 2009/05/03 21:34:26, Benjamin wrote:
> There's a small white space problem here.

Sorry, I don't see it: what is the problem?

http://codereview.appspot.com/52095/diff/1/13#newcode548
Line 548: if (lock && o->ob_type->tp_as_buffer->bf_getbuffer(o, NULL, 0)
< 0)
On 2009/05/03 21:34:26, Benjamin wrote:
> I believe you want PyObject_GetBuffer here.

Done. Notice, however, the loss of symmetry with release_bytes, where an
equivalent API function does not appear to exist.

http://codereview.appspot.com/52095/diff/1/13#newcode596
Line 596: bytes2str(name, 0));
On 2009/05/03 21:34:26, Benjamin wrote:
> What if byte2str returns NULL?

It really shouldn't; bytes2str should only be applied to results of the
PyUnicode_FSConverter, which should have checked that the result is
either bytes or bytearray. I have now changed bytes2str to give a
FatalError otherwise.

http://codereview.appspot.com/52095/diff/1/2
File Python/codecs.c (right):

http://codereview.appspot.com/52095/diff/1/2#newcode852
Line 852: if (!res)
On 2009/05/03 21:34:26, Benjamin wrote:
> This leaks "object".

Done. Again :-(

http://codereview.appspot.com/52095/diff/1/2#newcode895
Line 895: return NULL;
On 2009/05/03 21:34:26, Benjamin wrote:
> This leaks "object".

Done.

Please review this at http://codereview.appspot.com/52095

Affected files:
   M     Doc/library/codecs.rst
   M     Doc/library/os.rst
   M     Include/unicodeobject.h
   A     Lib/test/test_pep383.py
   M     Modules/_io/fileio.c
   M     Modules/posixmodule.c
   M     Modules/python.c
   M     Objects/unicodeobject.c
   M     Python/codecs.c
   M     Python/pythonrun.c
   M     configure
   M     configure.in
   M     pyconfig.h.in

----------

_______________________________________
Python tracker <report at bugs.python.org>
<http://bugs.python.org/issue5915>
_______________________________________