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-41137: Use utf-8 encoding while reading .pdbrc files #21263

Merged
merged 7 commits into from
Jul 8, 2021
Merged

bpo-41137: Use utf-8 encoding while reading .pdbrc files #21263

merged 7 commits into from
Jul 8, 2021

Conversation

srinivasreddy
Copy link
Contributor

@srinivasreddy srinivasreddy commented Jul 1, 2020

Copy link
Contributor

@eamanu eamanu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch :)

@serhiy-storchaka
Copy link
Member

Thank you for your PR @srinivasreddy, but it needs more changes:

  1. In the pdb documentation specify explicitly the encoding of the .pdbrc file.
  2. Add a note in What's New.
  3. Update tests. It would be nice to have a test failing with non-UTF-8 encoding.

@srinivasreddy
Copy link
Contributor Author

@serhiy-storchaka Not sure why windows build is failing. Any idea why?

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not forget about What's New.

try:
with support.temp_cwd():
with open('.pdbrc', 'w') as f:
f.write("Fran\u00E7ais\n")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will not work on locales with encoding which does not allow to encode \u00E7. You need to find characters which are encodable with the locale encoding.

Also, it would be better to use more realistic example. Use a command which takes a non-ASCII argument and check that it works correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the build is passing. And I would like to keep this negative test case as it would, in utf-8 encoding gives "NameError: name 'Française' is not defined". Or do you still want me write the test you commented above?

Doc/library/pdb.rst Outdated Show resolved Hide resolved
Lib/test/test_pdb.py Outdated Show resolved Hide resolved
@@ -1448,6 +1447,40 @@ def test_readrc_homedir(self):
if save_home is not None:
os.environ["HOME"] = save_home

def test_read_pdbrc_with_ascii_encoding(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure this test case is worth enough.

Doc/library/pdb.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.10.rst Outdated Show resolved Hide resolved
Lib/test/test_pdb.py Outdated Show resolved Hide resolved
@methane methane merged commit 58248d9 into python:main Jul 8, 2021
@srinivasreddy srinivasreddy deleted the bpo-41137 branch August 18, 2021 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants