GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-99593: Add tests for Unicode C API (part 1) by serhiy-storchaka · Pull Request #99651 · python/cpython

Conversation

@serhiy-storchaka

@serhiy-storchaka serhiy-storchaka commented Nov 21, 2022 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Add tests for functions corresponding to the str class methods.

  • Issue: Add tests for Unicode C API #99593

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@serhiy-storchaka
pythongh-99593: Add tests for Unicode C API (part 1)
7f5362f 
Add tests for functions corresponding to the str class methods.
@serhiy-storchaka serhiy-storchaka added needs backport to 3.10 needs backport to 3.11 only security fixes labels Nov 21, 2022
@serhiy-storchaka
serhiy-storchaka requested a review from vstinner November 21, 2022 14:59
@bedevere-bot bedevere-bot mentioned this pull request Nov 21, 2022
Add tests for Unicode C API #99593
Closed
@bedevere-bot bedevere-bot added the awaiting core review label Nov 21, 2022
@serhiy-storchaka serhiy-storchaka mentioned this pull request Nov 23, 2022
gh-99593: Add tests for Unicode C API #99594
Closed
vstinner
vstinner reviewed Nov 24, 2022
View reviewed changes

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

Very nice! Here is my first review :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_capi/test_unicode.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_capi/test_unicode.py Outdated
self.assertRaises(ValueError, split, 'a|b|c|d', '')
self.assertRaises(TypeError, split, 'a|b|c|d', ord('|'))
self.assertRaises(TypeError, split, [], '|')
# split(NULL, '|')

@vstinner vstinner Nov 24, 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.

what does this comment stand for? Does the function crash with NULL? Same question for similar rsplit() comment below.

Sorry, something went wrong.

Uh oh!

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

@serhiy-storchaka serhiy-storchaka Nov 27, 2022

Copy link
Copy Markdown
Member Author

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 crashes. It was the first test written by me 4 years ago, before I lost my sign, so I missed to add word CRASHES here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_capi/test_unicode.py
self.assertEqual(translate('abcd', {ord('a'): 'A', ord('b'): ord('B'), ord('c'): '<>'}), 'AB<>d')
self.assertEqual(translate('абвг', {ord('а'): 'А', ord('б'): ord('Б'), ord('в'): '<>'}), 'АБ<>г')
self.assertEqual(translate('abc', []), 'abc')
self.assertRaises(UnicodeTranslateError, translate, 'abc', {ord('b'): None})

@vstinner vstinner Nov 24, 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.

I don't understand. None is supposed to delete the "b" character: https://docs.python.org/dev/library/stdtypes.html#text-sequence-type-str

The mapping table must map Unicode ordinal integers to Unicode ordinal integers or None (causing deletion of the character).

Is the doc wrong?

Sorry, something went wrong.

Uh oh!

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

@serhiy-storchaka serhiy-storchaka Nov 27, 2022

Copy link
Copy Markdown
Member Author

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 doc is wrong.

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner Nov 28, 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.

Ah. The surprising part is that str.translate() treats None as "delete:

>>> "abc".translate(str.maketrans({'b': None}))
'ac'

Well, it would be nice to update the doc (maybe in a separated PR).

Sorry, something went wrong.

Uh oh!

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

@serhiy-storchaka serhiy-storchaka Nov 29, 2022

Copy link
Copy Markdown
Member Author

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.

Because str.translate calls PyUnicode_Translate() with the error handler "ignore".

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_capi/test_unicode.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_capi/test_unicode.py Outdated
#for str in "\xa1", "\u8000\u8080", "\ud800\udc02", "\U0001f100\U0001f1f1":
#for i, ch in enumerate(str):
#self.assertEqual(tailmatch(str, ch, 0, len(str), 1), i)
#self.assertEqual(tailmatch(str, ch, 0, len(str), -1), i)

@vstinner vstinner Nov 24, 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.

why is this code commented? if it is meaningless for tailmatch, just remove it?

Sorry, something went wrong.

Uh oh!

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

@serhiy-storchaka serhiy-storchaka Nov 27, 2022

Copy link
Copy Markdown
Member Author

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.

I copied it from other tests (for find/index/count), but did not adapted it to tailmatch yet. I think it is easier to remove it now.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_capi/test_unicode.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_capi/test_unicode.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka commented Nov 27, 2022
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member Author

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.

Thank you for your review Victor. I have a problem with reviewing such large volume of code, especially if many lines looks similar, so I can easily miss some types of errors. Without your help I would not find them.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_capi/test_unicode.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_capi/test_unicode.py Outdated
self.assertRaises(ValueError, split, 'a|b|c|d', '')
self.assertRaises(TypeError, split, 'a|b|c|d', ord('|'))
self.assertRaises(TypeError, split, [], '|')
# split(NULL, '|')

@serhiy-storchaka serhiy-storchaka Nov 27, 2022

Copy link
Copy Markdown
Member Author

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 crashes. It was the first test written by me 4 years ago, before I lost my sign, so I missed to add word CRASHES here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_capi/test_unicode.py
self.assertEqual(translate('abcd', {ord('a'): 'A', ord('b'): ord('B'), ord('c'): '<>'}), 'AB<>d')
self.assertEqual(translate('абвг', {ord('а'): 'А', ord('б'): ord('Б'), ord('в'): '<>'}), 'АБ<>г')
self.assertEqual(translate('abc', []), 'abc')
self.assertRaises(UnicodeTranslateError, translate, 'abc', {ord('b'): None})

@serhiy-storchaka serhiy-storchaka Nov 27, 2022

Copy link
Copy Markdown
Member Author

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 doc is wrong.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_capi/test_unicode.py Outdated
#for str in "\xa1", "\u8000\u8080", "\ud800\udc02", "\U0001f100\U0001f1f1":
#for i, ch in enumerate(str):
#self.assertEqual(tailmatch(str, ch, 0, len(str), 1), i)
#self.assertEqual(tailmatch(str, ch, 0, len(str), -1), i)

@serhiy-storchaka serhiy-storchaka Nov 27, 2022

Copy link
Copy Markdown
Member Author

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.

I copied it from other tests (for find/index/count), but did not adapted it to tailmatch yet. I think it is easier to remove it now.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_capi/test_unicode.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_capi/test_unicode.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_capi/test_unicode.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka
Address review comments.
545400a
vstinner
vstinner approved these changes Nov 28, 2022
View reviewed changes

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_capi/test_unicode.py
self.assertEqual(translate('abcd', {ord('a'): 'A', ord('b'): ord('B'), ord('c'): '<>'}), 'AB<>d')
self.assertEqual(translate('абвг', {ord('а'): 'А', ord('б'): ord('Б'), ord('в'): '<>'}), 'АБ<>г')
self.assertEqual(translate('abc', []), 'abc')
self.assertRaises(UnicodeTranslateError, translate, 'abc', {ord('b'): None})

@vstinner vstinner Nov 28, 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.

Ah. The surprising part is that str.translate() treats None as "delete:

>>> "abc".translate(str.maketrans({'b': None}))
'ac'

Well, it would be nice to update the doc (maybe in a separated PR).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Nov 28, 2022
@serhiy-storchaka
serhiy-storchaka merged commit deaa8de into python:main Nov 29, 2022
@miss-islington

miss-islington commented Nov 29, 2022

Copy link
Copy Markdown
Contributor

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting merge label Nov 29, 2022
@miss-islington

miss-islington commented Nov 29, 2022

Copy link
Copy Markdown
Contributor

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker deaa8dee48beeae9928a418736da0608f2f18361 3.11
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington miss-islington assigned serhiy-storchaka Nov 29, 2022
@miss-islington

miss-islington commented Nov 29, 2022

Copy link
Copy Markdown
Contributor

Sorry @serhiy-storchaka, I had trouble checking out the 3.10 backport branch.
Please retry by removing and re-adding the "needs backport to 3.10" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker deaa8dee48beeae9928a418736da0608f2f18361 3.10
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka serhiy-storchaka mentioned this pull request Nov 29, 2022
gh-93649: Split unicode tests from _testcapimodule.c & add some more #95819
Merged
@vstinner

vstinner commented Nov 29, 2022

Copy link
Copy Markdown
Member

Oh, I didn't notice that you want to backport these tests to Python 3.10 and 3.11. You're motivated :-) If it's too complicated, maybe just add them to Python 3.12, no? _testcapi changed a lot since Python 3.11 (splited into multiple files).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka

serhiy-storchaka commented Nov 29, 2022

Copy link
Copy Markdown
Member Author

I think that we should backport as many tests as possible, otherwise we risk to miss a regression introduced before the particular test was added. Especially if we do so many changes in C API.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka serhiy-storchaka added needs backport to 3.11 only security fixes and removed needs backport to 3.10 needs backport to 3.11 only security fixes labels Jul 10, 2023
@miss-islington

miss-islington commented Jul 10, 2023

Copy link
Copy Markdown
Contributor

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington

miss-islington commented Jul 10, 2023

Copy link
Copy Markdown
Contributor

Sorry @serhiy-storchaka, I had trouble checking out the 3.11 backport branch.
Please retry by removing and re-adding the "needs backport to 3.11" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker deaa8dee48beeae9928a418736da0608f2f18361 3.11
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka
serhiy-storchaka deleted the test-unicode-capi5 branch July 10, 2023 13:08
@serhiy-storchaka serhiy-storchaka removed their assignment Jul 10, 2023
@serhiy-storchaka serhiy-storchaka removed the needs backport to 3.11 only security fixes label Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

4 participants

@serhiy-storchaka @miss-islington @vstinner @bedevere-bot
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.