bpo-29783: Replace codecs.open() with io.open() #599

vstinner · 2017-03-10T14:44:13Z

In test_sax, remove buffering=0 since io.open() doesn't support
unbuffered text I/O.

mention-bot · 2017-03-10T14:44:18Z

@Haypo, thanks for your PR! By analyzing the history of the files in this pull request, we identified @loewis, @benjaminp, @asvetlov, @birkenfeld and @doerwalter to be potential reviewers.

serhiy-storchaka

Some changes LGTM, but others are not. Changes to codecs and test_codecs look unrelated to other changes and to the purpose of this PR.

serhiy-storchaka · 2017-03-10T14:49:14Z

Doc/tools/extensions/pyspecific.py

@@ -254,11 +254,8 @@ def run(self):
        fpath = path.join(source_dir, fname)
        self.state.document.settings.record_dependencies.add(fpath)
        try:
-            fp = codecs.open(fpath, encoding='utf-8')
-            try:
+            with io.open(fpath, encoding='utf-8') as fp:


Why not builtin open?

Maybe some people or services still use Python 2 to build the Python documentation, no?

Ah, I missed that this is not a part of the stdlib.

serhiy-storchaka · 2017-03-10T14:50:45Z

Lib/lib2to3/refactor.py

@@ -107,22 +107,6 @@ def get_fixers_from_package(pkg_name):
 def _identity(obj):
    return obj

-if sys.version_info < (3, 0):


This looks as a red flag. Should this file be compatible with Python 2?

io.open() is available since Python 2.6, so my change should work on Python 2.6 and newer.

I don't know why this file has support for Python 2.

Agree, we can drop the support of Python 2.5.

serhiy-storchaka · 2017-03-10T14:54:24Z

Lib/test/test_multibytecodec.py

+            with open(TESTFN, 'wb') as fp:
+                fp.write(b'\xa1')
+
+            with open(TESTFN, encoding='cp949') as fp:


I afraid this invalidates the purpose of the test.

bug1728403 originally was about codecs.open(). I don't know if there is a sense to use io.open() here.

serhiy-storchaka · 2017-03-10T14:56:03Z

Lib/test/test_sax.py

@@ -774,8 +774,8 @@ class StreamReaderWriterXmlgenTest(XmlgenTest, unittest.TestCase):
    fname = support.TESTFN + '-codecs'

    def ioclass(self):
-        writer = codecs.open(self.fname, 'w', encoding='ascii',
-                             errors='xmlcharrefreplace', buffering=0)
+        writer = open(self.fname, 'w', encoding='ascii',


This invalidates the purpose of the test. This is the test for StreamReaderWriter.

I don't understand why test_sax tests the StreamReaderWriter class.

It seems like io.TextIOWrapper is not tested, whereas it's more common than codecs.StreamReaderWriter.

The code of XMLGenerator is complex, it contains a lot of special cases and should be tested for different kinds of writers.

It would be better to add a test for io.TextIOWrapper (if testing io.StringIO is not enough), but keep the test for StreamReaderWriter.

Keep the existing code. This test is purposed for testing StreamReaderWriter.

vstinner · 2017-06-14T21:21:37Z

I simpiified the PR to only keep the less controversal changes.

@serhiy-storchaka: would you mind to review this shorted and rebased PR please?

serhiy-storchaka

Some tests should use codecs streams.

serhiy-storchaka · 2017-06-15T04:37:54Z

Lib/test/test_multibytecodec.py

+            with open(TESTFN, 'wb') as fp:
+                fp.write(b'\xa1')
+
+            with open(TESTFN, encoding='cp949') as fp:


bug1728403 originally was about codecs.open(). I don't know if there is a sense to use io.open() here.

serhiy-storchaka · 2017-06-15T04:39:35Z

Lib/test/test_sax.py

@@ -774,8 +774,8 @@ class StreamReaderWriterXmlgenTest(XmlgenTest, unittest.TestCase):
    fname = support.TESTFN + '-codecs'

    def ioclass(self):
-        writer = codecs.open(self.fname, 'w', encoding='ascii',
-                             errors='xmlcharrefreplace', buffering=0)
+        writer = open(self.fname, 'w', encoding='ascii',


Keep the existing code. This test is purposed for testing StreamReaderWriter.

vstinner · 2017-06-15T23:04:31Z

I reverted changes in Lib/test/test_multibytecodec.py and Lib/test/test_sax.py.

serhiy-storchaka

The remaining cosmetic changes LGTM.

serhiy-storchaka reviewed Mar 10, 2017

View reviewed changes

serhiy-storchaka requested a review from malemburg March 10, 2017 15:26

serhiy-storchaka requested changes Jun 15, 2017

View reviewed changes

bpo-29783: Replace codecs.open() with io.open()

b33713d

serhiy-storchaka approved these changes Jun 16, 2017

View reviewed changes

vstinner merged commit 272d888 into python:master Jun 16, 2017

vstinner deleted the codecs_open2 branch June 16, 2017 06:59

Uh oh!

bpo-29783: Replace codecs.open() with io.open() #599

bpo-29783: Replace codecs.open() with io.open() #599

Uh oh!

Conversation

vstinner commented Mar 10, 2017

Uh oh!

mention-bot commented Mar 10, 2017

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Jun 14, 2017

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Jun 15, 2017

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!