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-103997: Fix and test `_PyUnicode_Dedent` by StanFromIreland · Pull Request #138620 · python/cpython

Conversation

@StanFromIreland

@StanFromIreland StanFromIreland commented Sep 7, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

While working on #62535, I noticed that several textwrap.dedent tests fail with this implementation.

  • Issue: Auto dedent -c arguments #103997

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@StanFromIreland
Commit
40bcdea
@StanFromIreland
StanFromIreland requested a review from methane September 7, 2025 12:48
@bedevere-app bedevere-app Bot added the awaiting review label Sep 7, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Sep 7, 2025
Auto dedent -c arguments #103997
Closed
picnixz
picnixz requested changes Sep 7, 2025
View reviewed changes

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

_PyUnicode_Dedent is used in pymain_run_command so it may be performance critical. So please keep the same logic for implementation by working with char* only. Or show that this doesn't result in a performance loss.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Include/internal/pycore_unicodeobject.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/unicodeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added awaiting changes and removed awaiting review labels Sep 7, 2025
@bedevere-app

This comment was marked as resolved.

Sign in to view
@StanFromIreland
Add comment & un-refactor
f6ace9d
@StanFromIreland

This comment was marked as resolved.

Sign in to view
@bedevere-app bedevere-app Bot added awaiting change review and removed awaiting changes labels Sep 7, 2025
@bedevere-app

This comment was marked as resolved.

Sign in to view
@bedevere-app
bedevere-app Bot requested a review from picnixz September 7, 2025 15:07
picnixz
picnixz requested changes Sep 7, 2025
View reviewed changes

@picnixz picnixz 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 avoid adding const qualifiers.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/unicodeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the awaiting change review label Sep 7, 2025
@bedevere-app

This comment was marked as resolved.

Sign in to view
@bedevere-app bedevere-app Bot added the awaiting changes label Sep 7, 2025
@StanFromIreland
Revert some more
3d3d957
@StanFromIreland

This comment was marked as resolved.

Sign in to view
@bedevere-app bedevere-app Bot added awaiting change review and removed awaiting changes labels Sep 7, 2025
@bedevere-app

This comment was marked as resolved.

Sign in to view
@bedevere-app
bedevere-app Bot requested a review from picnixz September 7, 2025 15:13
picnixz
picnixz requested changes Sep 7, 2025
View reviewed changes
Comment thread Objects/unicodeobject.c Outdated
Py_ssize_t whitespace_len = search_longest_common_leading_whitespace(
src, end, &whitespace_start);

if (whitespace_len == 0) {

@picnixz picnixz Sep 7, 2025

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.

Keep the fast path.

Sorry, something went wrong.

Uh oh!

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

@StanFromIreland StanFromIreland Sep 7, 2025 •
edited
Loading

Uh oh!

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

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.

We can't, we need to clear lines, see the tests.

Sorry, something went wrong.

Uh oh!

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

@picnixz picnixz Sep 7, 2025

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.

Do we really need to do it? in general there is nothing to dedent, so it'll slow down normal cases. Even if the comment says that it's meant to match textwrap.dedent(), I don't think it's needed.

Sorry, something went wrong.

Uh oh!

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

@StanFromIreland StanFromIreland Sep 7, 2025

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.

If we want to respect the behavior of textwrap.dedent, as is noted in What's new 3.14, the docs, and the comments. Then yes.

Sorry, something went wrong.

Uh oh!

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

@picnixz picnixz Sep 7, 2025 •
edited
Loading

Uh oh!

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

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.

But I don't think we need to! It's an internal function. textwrap.dedent is implemented in pure Python.

Sorry, something went wrong.

Uh oh!

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

@StanFromIreland StanFromIreland Sep 7, 2025 •
edited
Loading

Uh oh!

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

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.

But our documentation says we do:

Whats New 3.14

The auto-dedentation behavior mirrors textwrap.dedent().

Sorry, something went wrong.

Uh oh!

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

@StanFromIreland StanFromIreland Sep 7, 2025

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.

If you would rather just remove the false claims, we can do: https://github.com/python/cpython/compare/main...StanFromIreland:remove-misleading-notes?expand=1

Though I think this may cause confusion in the future.

Sorry, something went wrong.

Uh oh!

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

@picnixz picnixz Sep 7, 2025 •
edited
Loading

Uh oh!

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

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 auto-dedentation behavior mirrors textwrap.dedent().

Yes, but we can just say that we don't normalize whitespaces and only consider spaces and tabs. No one should ever write spaces with other space-like characters. Let's just amend the NEWS. As for str.dedent(), it will need a PEP which still doesn't exist and the discussion seems stalled IMO.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
methane reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@StanFromIreland StanFromIreland Sep 7, 2025

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.

Yes, I am working on the PEP :-)

Sorry, something went wrong.

Uh oh!

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

@StanFromIreland StanFromIreland Sep 7, 2025

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 opened #138620 with your alternative, though I still think fixing it is better.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/unicodeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/unicodeobject.c Outdated
// if this line has all white space, write '\n' and continue
if (in_leading_space && append_newline) {
*dest_iter++ = '\n';
if (in_leading_space) {

@picnixz picnixz Sep 7, 2025

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.

Was this the issue? or was it *iter != ' ' ...?

Sorry, something went wrong.

Uh oh!

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

@StanFromIreland StanFromIreland Sep 7, 2025

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.

There were multiple issues.

Sorry, something went wrong.

Uh oh!

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

@picnixz picnixz Sep 7, 2025

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 indicate the issues for posterity.

Sorry, something went wrong.

Uh oh!

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

@StanFromIreland StanFromIreland Sep 7, 2025

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.

There are two issues:

  1. Not clearing lines that are only whitespace, whereas textwrap.dedent does
  2. Only considering '\t' and ' ', whereas textwrap.dedent uses str.isspace

Sorry, something went wrong.

Uh oh!

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

@picnixz picnixz Sep 7, 2025

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.

Honestly, I don't think it's worth changing this function. We should just change the comment. It's an internal function.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot removed the awaiting change review label Sep 7, 2025
@bedevere-app

This comment was marked as resolved.

Sign in to view
@bedevere-app bedevere-app Bot added the awaiting changes label Sep 7, 2025
@picnixz

picnixz commented Sep 7, 2025

Copy link
Copy Markdown
Member

I would still be interested in knowing the answer to that question:

Or show that this doesn't result in a performance loss

Did your refactoring improve the overall performance or not?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@StanFromIreland
Revert odd line removal
4403936
@StanFromIreland

StanFromIreland commented Sep 7, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

Did your refactoring improve the overall performance or not?

Using PyUnicodeWriter has a ~20% performance penalty.
All reactions

Sorry, something went wrong.

Uh oh!

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

StanFromIreland commented Sep 7, 2025

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added awaiting change review and removed awaiting changes labels Sep 7, 2025
@bedevere-app

This comment was marked as resolved.

Sign in to view
@bedevere-app
bedevere-app Bot requested a review from picnixz September 7, 2025 15:24
picnixz
picnixz reviewed Sep 7, 2025
View reviewed changes
Comment thread Include/internal/pycore_unicodeobject.h 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 Modules/_testinternalcapi.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_testinternalcapi.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/unicodeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/unicodeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/unicodeobject.c Outdated
Py_ssize_t whitespace_len = search_longest_common_leading_whitespace(
src, end, &whitespace_start);

if (whitespace_len == 0) {

@picnixz picnixz Sep 7, 2025

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.

Do we really need to do it? in general there is nothing to dedent, so it'll slow down normal cases. Even if the comment says that it's meant to match textwrap.dedent(), I don't think it's needed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/unicodeobject.c Outdated
// if this line has all white space, write '\n' and continue
if (in_leading_space && append_newline) {
*dest_iter++ = '\n';
if (in_leading_space) {

@picnixz picnixz Sep 7, 2025

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 indicate the issues for posterity.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@StanFromIreland
Changes
0cf91f2
@StanFromIreland StanFromIreland mentioned this pull request Sep 7, 2025
gh-103997: Remove incorrect statements about -c dedenting #138624
Merged
@picnixz

picnixz commented Sep 7, 2025

Copy link
Copy Markdown
Member

We can still test _PyUnicode_Dedent to check that it matches a "subset" of the features of textwrap.dedent.
All reactions

Sorry, something went wrong.

Uh oh!

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

sunmy2019 commented Oct 15, 2025

Copy link
Copy Markdown
Member

I'm the implementor of the C function. Sorry that I did not read the code of textwrap.dedent. I just wrote according to the test cases. 😳

I understand now it's a matter of design.
All reactions

Sorry, something went wrong.

Uh oh!

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

picnixz commented Oct 16, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Yes, and I think we do not need to exactly mimic textwrap dedent unless there is a compelling reason. I personally do not find one: the function is private and internally used only by the parser I think, and I doubt anyone would have a script that uses whitespaces that are not spaces/tabs for indents, and if they do, I do not think we should support this at the cost of slowing down the regular use cases.

For instance, I would suggest that we currently keep a simplified version as it is only used for the parser and, if the PEP for str.dedent() is accepted, possibly revisit this design question later, possibly by adding support for normalisation as well (PyUnicode_Dedent and PyUnicode_DedentNormalize).

WDYT?
All reactions

Sorry, something went wrong.

Uh oh!

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

sunmy2019 commented Oct 16, 2025

Copy link
Copy Markdown
Member

For instance, I would suggest that we currently keep a simplified version as it is only used for the parser and, if the PEP for str.dedent() is accepted, possibly revisit this design question later, possibly by adding support for normalisation as well (PyUnicode_Dedent and PyUnicode_DedentNormalize).

WDYT?

I think so. Let's just change the document for now.
All reactions

Sorry, something went wrong.

Uh oh!

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

StanFromIreland commented Mar 7, 2026

Copy link
Copy Markdown
Member Author

@methane a little ping, could you please review this or #138624?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@StanFromIreland
Merge branch 'main' into fix-dedent
6da0dd1
@methane

methane commented Mar 7, 2026 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

I'm +1 to just fix document for now.

I'm not sure if it's correct that textwrap.dedent() treats all Unicode whitespace characters as indentation characters.
Furthermore, python -c only dedents the indentation of Python code.
It's not too late to treat Unicode whitespace characters as indentation characters after the PEP for str.dedent() is accepted.
StanFromIreland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@StanFromIreland StanFromIreland closed this Mar 7, 2026
@StanFromIreland
StanFromIreland deleted the fix-dedent branch March 7, 2026 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@picnixz picnixz picnixz requested changes
@methane methane Awaiting requested review from methane

Uh oh!

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

Assignees

No one assigned

Labels

awaiting change review

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

@StanFromIreland @picnixz @sunmy2019 @methane
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.