GeezerVison

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

GeezerVison is showing github.com — gh-123682: Fix Unicode category check by SnoopJ · Pull Request #123683 · python/cpython

Conversation

SnoopJ commented Sep 4, 2024 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

This changeset fixes the faulty predicate identified by #123682 when testing if an input character is in one of the Unicode Other categories

I assume this doesn't need a news fragment, but if I'm wrong, let me know and I'll add one.
(Sorry about the force-push chatter, I started this branch from an unrelated commit 😬)

Issue: _pyrepl.input.KeymapTranslator tests for non-existent Unicode category #123682

Sorry, something went wrong.

Uh oh!

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

All reactions

SnoopJ requested review from ambv, lysnikolaou and pablogsal as code owners

September 4, 2024 16:39

bedevere-app Bot added the awaiting review label

Sep 4, 2024

SnoopJ force-pushed the bugfix/gh-123682_tweak-pyrepl-unicodedata-usage branch from 495bb25 to 52fa7be Compare

September 4, 2024 16:42


          Fix Unicode category check

0ab70cf

SnoopJ force-pushed the bugfix/gh-123682_tweak-pyrepl-unicodedata-usage branch from 52fa7be to 0ab70cf Compare

September 4, 2024 16:42

vstinner previously approved these changes

Sep 5, 2024

View reviewed changes

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

LGTM.

cc @ambv

Sorry, something went wrong.

Uh oh!

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

All reactions

bedevere-app Bot added awaiting merge and removed awaiting review labels

Sep 5, 2024

vstinner added needs backport to 3.13

only security fixes

topic-repl Related to the interactive shell labels

Sep 5, 2024

vstinner commented Sep 6, 2024

Copy link

Copy Markdown

Member

Ah, you have to update test_pyrepl:

ERROR: test_control_character (test.test_pyrepl.test_pyrepl.TestPyReplOutput.test_control_character)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/test_pyrepl/test_pyrepl.py", line 657, in test_control_character
    output = multiline_input(reader)
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/test_pyrepl/support.py", line 18, in multiline_input
    return reader.readline()
           ~~~~~~~~~~~~~~~^^
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/_pyrepl/reader.py", line 786, in readline
    self.handle1()
    ~~~~~~~~~~~~^^
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/_pyrepl/reader.py", line 743, in handle1
    event = self.console.get_event(block)
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/test_pyrepl/support.py", line 123, in get_event
    return next(self.events)
StopIteration

======================================================================
FAIL: test_control_characters (test.test_pyrepl.test_reader.TestReader.test_control_characters)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/test_pyrepl/test_reader.py", line 126, in test_control_characters
    self.assert_screen_equals(reader, 'flag = "🏳️\\u200d🌈"')
    ~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/test_pyrepl/test_reader.py", line 16, in assert_screen_equals
    self.assertListEqual(actual, expected)
    ~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^
AssertionError: Lists differ: ['flag = "🏳️🌈"'] != ['flag = "🏳️\\u200d🌈"']

First differing element 0:
'flag = "🏳️🌈"'
'flag = "🏳️\\u200d🌈"'

- ['flag = "🏳️🌈"']
+ ['flag = "🏳️\\u200d🌈"']
?             +++++++

All reactions

Sorry, something went wrong.

Uh oh!

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

SnoopJ commented Sep 6, 2024 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor Author

Oops! Hmm, I think that needs more attention than just in the tests.

The second failure is because the ZWJ is in category Cf and is being erroneously treated as invalid. If I change the predicate to test only for Cc membership, that passes.

The first failure looks more serious. The proximate cause is (I think) the missing newline (also in Cc) but an ad-hoc tweak to the predicate to try and let the newline pass through still fails.

I think overall this means that a category check might not be the right way to express the intent of the original code. @pablogsal, do you mind chiming in here? My understanding of this check is that it's meant to filter out control characters like what's in C0 and C1, but it seems like some of those characters are not wanted in this predicate, so just testing ord(key) <= 0x1F wouldn't be appropriate either. The \x1d in the test should definitely pass through, for instance.

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner dismissed their stale review

September 8, 2024 11:20

tests fail, the current change looks wrong in fact

bedevere-app Bot added awaiting review and removed awaiting merge labels

Sep 8, 2024

serhiy-storchaka added the needs backport to 3.14 bugs and security fixes label

May 8, 2025

bedevere-app Bot mentioned this pull request

Nov 29, 2025

_pyrepl.input.KeymapTranslator tests for non-existent Unicode category #123682

Open


          Merge branch 'main' into bugfix/gh-123682_tweak-pyrepl-unicodedata-usage

a913d26

github-actions Bot commented Apr 18, 2026

Copy link

Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

All reactions

Sorry, something went wrong.

Uh oh!

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

github-actions Bot added the stale Stale PR or inactive for long period of time. label

Apr 18, 2026

serhiy-storchaka added the needs backport to 3.15 bugs and security fixes label

May 30, 2026

github-actions Bot removed the stale Stale PR or inactive for long period of time. label

May 30, 2026

github-actions Bot commented Sep 1, 2026

Copy link

Copy Markdown

This PR is stale because it has been open for 90 days with no activity.

All reactions

Sorry, something went wrong.

Uh oh!

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

github-actions Bot added the stale Stale PR or inactive for long period of time. label

Sep 1, 2026

Reviewers

vstinner

vstinner left review comments

pablogsal

Awaiting requested review from pablogsal

pablogsal is a code owner

lysnikolaou

Awaiting requested review from lysnikolaou

lysnikolaou is a code owner

ambv

Awaiting requested review from ambv

ambv is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

awaiting review needs backport to 3.13

only security fixes

needs backport to 3.14

bugs and security fixes

needs backport to 3.15

bugs and security fixes

stale

Stale PR or inactive for long period of time.

topic-repl

Related to the interactive shell

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.

3 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

SnoopJ commented Sep 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Sep 6, 2024

Uh oh!

SnoopJ commented Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Apr 18, 2026

Uh oh!

github-actions Bot commented Sep 1, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SnoopJ commented Sep 4, 2024 •

edited

Loading

Uh oh!

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

SnoopJ commented Sep 6, 2024 •

edited

Loading

Uh oh!

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