bpo-37367: octal escapes applied inconsistently throughout the interpreter and lib#14654
bpo-37367: octal escapes applied inconsistently throughout the interpreter and lib#14654websurfer5 wants to merge 2 commits intopython:mainfrom
Conversation
values greater than 255 (0o377)
jdemeyer
left a comment
There was a problem hiding this comment.
Looks good, apart from a few nitpicks.
Misc/NEWS.d/next/Core and Builtins/2019-07-08-19-08-38.bpo-37367.SVtBhh.rst
Outdated
Show resolved
Hide resolved
1. use "octal escape sequences" instead of "octal values" in comments 2. remove underscore from _literal_eval Also, use "0o1000-0o400" instead of "256" to make it clear to the reader where the value came from.
| @@ -0,0 +1,2 @@ | |||
| Byte strings containing octal escape sequences with values greater than 255 | |||
| will now raise ValueError. Patch by Jeffrey Kintscher. | |||
There was a problem hiding this comment.
It would be helpful to mention the previous behavior here.
| import tempfile | ||
| import textwrap | ||
| import unittest | ||
| import ast |
There was a problem hiding this comment.
Please keep the imports sorted alphabetically.
|
@websurfer5, could you merge the master branch into this PR's branch, resolving the merge conflicts? |
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Please add also a not in the What's New document. Since it is a breaking change it should be in section "Porting to Python 3.11".
| c = b.translate(None, delete=b'e') | ||
| self.assertEqual(c, b'hllo') | ||
|
|
||
| def test_octal_values(self): |
There was a problem hiding this comment.
It is not correct place for such test. This file is for tests of bytes and bytearray methods, not parsing Python code.
test_string_literals is for parsing string and bytes literals. Add some tests there.
| check(br"[\101]", b"[A]") | ||
| check(br"[\1010]", b"[A0]") | ||
| check(br"[\501]", b"[A]") | ||
| self.assertRaises(ValueError, decode, br"[\501]") |
There was a problem hiding this comment.
Move it to test_errors below.
| } | ||
| if (c > 255) { | ||
| PyErr_Format(PyExc_ValueError, | ||
| "octal value must be in range(0, 256)"); |
There was a problem hiding this comment.
The error message should contain the position of the invalid octal escape sequence. It would be nice if look similar to error message for invalid hexadecimal escape sequence. For example:
"invalid octal escape sequence at position %zd"
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
#91668 was merged instead. |
Add a range check for octal values in byte strings that raises ValueError if the value is more than 255 (0o377).
Tests for all 1-, 2-, and 3-digit octal values are provided (both valid and error cases).
https://bugs.python.org/issue37367