Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-36778: cp65001 encoding becomes an alias to utf_8 #13230

Merged
merged 1 commit into from May 10, 2019

Conversation

@vstinner
Copy link
Member

commented May 9, 2019

@vstinner

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

I reproduced #13110 (comment) benchmark:

Mean +- std dev: [ref] 156 ns +- 3 ns -> [remove] 105 ns +- 3 ns: 1.48x faster (-32%)

@vstinner vstinner requested a review from methane May 9, 2019

@vstinner

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

@methane: Are you ok to simply remove cp65001.py?

@paulmon

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

I can verify that this change fixes the issue with test_startup_imports I found on Windows IoT Core ARM32 as expected.

Also, all test_site and test_codec tests pass on Windows IoT Core with a default codepage of 65001

@methane
Copy link
Member

left a comment

@methane: Are you ok to simply remove cp65001.py?

Yes.

@vstinner vstinner merged commit d267ac2 into python:master May 10, 2019

5 checks passed

Azure Pipelines PR #20190509.54 succeeded
Details
bedevere/issue-number Issue number 36778 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@vstinner vstinner deleted the vstinner:remove_cp65001 branch May 10, 2019

@@ -1106,8 +1106,7 @@ particular, the following variants typically exist:
+-----------------+--------------------------------+--------------------------------+
| cp1258 | windows-1258 | Vietnamese |
+-----------------+--------------------------------+--------------------------------+
| cp65001 | | Windows only: Windows UTF-8 |
| | | (``CP_UTF8``) |
| cp65001 | | Alias to ``utf_8`` encoding |

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka May 10, 2019

Member

Add the versionchanged directive.

I think it is better to remove this row add add cp65001 to the list of utf-8 aliases.

This comment has been minimized.

Copy link
@vstinner

vstinner May 10, 2019

Author Member

I created PR #13240 for your doc change proposal.

@@ -875,95 +875,6 @@ def test_surrogatepass_handler(self):
b"abc\xed\xa0z".decode(self.encoding, "surrogatepass")


@unittest.skipUnless(sys.platform == 'win32',
'cp65001 is a Windows-only codec')
class CP65001Test(ReadTest, unittest.TestCase):

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka May 10, 2019

Member

Does utf-8 pass these tests?

This comment has been minimized.

Copy link
@methane

methane May 10, 2019

Member

I confirmed utf-8 passes this test.

This comment has been minimized.

Copy link
@vstinner

vstinner May 10, 2019

Author Member

Does utf-8 pass these tests?

Sorry, I forgot to specify before I merged my PR that yes: I tested on my Windows 10 and the test still passed. But CP65001Test is now redundant with UTF8Ttest.

I confirmed utf-8 passes this test.

Thanks for checking :-)

@taleinat

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

Shouldn't this comment in CodePageTest in Lib/test/test_codecs.py also be removed? It no longer makes any sense.

class CodePageTest(unittest.TestCase):
    # CP_UTF8 is already tested by CP65001Test
    CP_UTF8 = 65001
@vstinner

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

Shouldn't this comment in CodePageTest in Lib/test/test_codecs.py also be removed? It no longer makes any sense.

Right. I wrote PR #13807 to remove the comment.

PatrikKopkan pushed a commit to PatrikKopkan/cpython that referenced this pull request Jul 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.