GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-37367: octal escapes applied inconsistently throughout the interpreter and lib by websurfer5 · Pull Request #14654 · python/cpython

Conversation

websurfer5 commented Jul 9, 2019 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          bpo-37367: raise ValueError for byte strings containing octal

b3459cb

values greater than 255 (0o377)

the-knights-who-say-ni added the CLA signed label

Jul 9, 2019

bedevere-bot added the awaiting review label

Jul 9, 2019

jdemeyer requested changes

Jul 9, 2019

View reviewed changes

jdemeyer left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Looks good, apart from a few nitpicks.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/test/test_bytes.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Lib/test/test_bytes.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Misc/NEWS.d/next/Core and Builtins/2019-07-08-19-08-38.bpo-37367.SVtBhh.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot added awaiting core review and removed awaiting review labels

Jul 9, 2019

brettcannon added the type-bug An unexpected behavior, bug, or error label

Jul 11, 2019


          bpo-37367: changes per reviewer request:

b521f71

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.

taleinat reviewed

Nov 17, 2019

View reviewed changes

Comment thread

Misc/NEWS.d/next/Core and Builtins/2019-07-08-19-08-38.bpo-37367.SVtBhh.rst

		@@ -0,0 +1,2 @@
		Byte strings containing octal escape sequences with values greater than 255
		will now raise ValueError. Patch by Jeffrey Kintscher.

taleinat Nov 17, 2019

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It would be helpful to mention the previous behavior here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka reacted with thumbs up emoji

All reactions

👍 1 reaction

taleinat reviewed

Nov 17, 2019

View reviewed changes

Comment thread

Lib/test/test_bytes.py

		import tempfile
		import textwrap
		import unittest
		import ast

taleinat Nov 17, 2019

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please keep the imports sorted alphabetically.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka reacted with thumbs up emoji

All reactions

👍 1 reaction

taleinat commented Nov 17, 2019

Copy link

Copy Markdown

Contributor

@websurfer5, could you merge the master branch into this PR's branch, resolving the merge conflicts?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka requested changes

Mar 19, 2022

View reviewed changes

serhiy-storchaka left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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".

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/test/test_bytes.py

		c = b.translate(None, delete=b'e')
		self.assertEqual(c, b'hllo')

		def test_octal_values(self):

serhiy-storchaka Mar 19, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/test/test_codecs.py

		check(br"[\101]", b"[A]")
		check(br"[\1010]", b"[A0]")
		check(br"[\501]", b"[A]")
		self.assertRaises(ValueError, decode, br"[\501]")

serhiy-storchaka Mar 19, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Move it to test_errors below.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Objects/bytesobject.c

		}
		if (c > 255) {
		PyErr_Format(PyExc_ValueError,
		"octal value must be in range(0, 256)");

serhiy-storchaka Mar 19, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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"

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-bot removed the awaiting core review label

Mar 19, 2022

bedevere-bot commented Mar 19, 2022

Copy link

Copy Markdown

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot added the awaiting changes label

Mar 19, 2022

mr-nfamous mannequin mentioned this pull request

Apr 18, 2022

octal escapes applied inconsistently throughout the interpreter and lib #81548

Closed

ezio-melotti removed the CLA signed label

Jul 13, 2022

serhiy-storchaka commented Dec 1, 2023

Copy link

Copy Markdown

Member

#91668 was merged instead.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka closed this

Dec 1, 2023

Reviewers

jdemeyer

jdemeyer requested changes

serhiy-storchaka

serhiy-storchaka requested changes

+1 more reviewer

taleinat

taleinat left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting changes type-bug

An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

websurfer5 commented Jul 9, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdemeyer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

taleinat Nov 17, 2019

Choose a reason for hiding this comment

Uh oh!

taleinat Nov 17, 2019

Choose a reason for hiding this comment

Uh oh!

taleinat commented Nov 17, 2019

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Mar 19, 2022

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Mar 19, 2022

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Mar 19, 2022

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Mar 19, 2022

Uh oh!

serhiy-storchaka commented Dec 1, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

websurfer5 commented Jul 9, 2019 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.