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 — bpo-24665: double-width CJK chars support for textwrap by fgallaire · Pull Request #89 · python/cpython

Conversation

@fgallaire

@fgallaire fgallaire commented Feb 14, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
  • Add ckj option flag, default to False
  • Add cjk_wide(), cjk_len() and cjk_slices() utilities

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@fgallaire
bpo-24665: double-width CJK chars support for textwrap
6ea78e3 
* Add ckj option flag, default to False
* Add cjkwide(), cjklen() and cjkslices() utilities
@the-knights-who-say-ni the-knights-who-say-ni added the CLA not signed label Feb 14, 2017
@the-knights-who-say-ni

the-knights-who-say-ni commented Feb 14, 2017

Copy link
Copy Markdown

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA. This is necessary for legal reasons before we can look at your contribution. Please follow these steps to help rectify the issue:

  1. If you don't have an account on b.p.o, please create one
  2. Make sure your GitHub username is listed in "Your Details" at b.p.o
  3. If you have not already done so, please sign the PSF contributor agreement
  4. If you just signed the CLA, please wait at least one US business day and then check "Your Details" on bugs.python.org to see if your account has been marked as having signed the CLA (the delay is due to a person having to manually check your signed CLA)
  5. Reply here saying you have completed the above steps

Thanks again to your contribution and we look forward to looking at it!
All reactions

Sorry, something went wrong.

Uh oh!

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

fgallaire commented Feb 14, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Author

I now have listed my GitHub username at bpo and i had already signed the CLA.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@zware zware removed the CLA not signed label Feb 14, 2017
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Feb 14, 2017
methane
methane requested changes Feb 14, 2017
View reviewed changes
Comment thread Lib/textwrap.py Outdated

def __init__(self,
width=70,
cjk=False,

@methane methane Feb 14, 2017

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.

These arguments can be passed as positional argument.
So new argument should be added last.

Sorry, something went wrong.

Uh oh!

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

@fgallaire fgallaire Feb 14, 2017

Copy link
Copy Markdown
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 agree

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/textwrap.py Outdated
i = 1
# <= and i-1 to catch the last double length char of odd line
while cjklen(text[:i]) <= index:
i = i + 1

@methane methane Feb 14, 2017

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 like this O(n^2) algorithm.

w = 0
for i, c in enumerate(text):
    w += cjkwide(c) + 1
    if w > index:
        break

Sorry, something went wrong.

Uh oh!

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

@fgallaire fgallaire Feb 14, 2017

Copy link
Copy Markdown
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.

Very relevant point.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@fgallaire
fgallaire force-pushed the master branch 2 times, most recently from e52db22 to 5c4b2af Compare February 14, 2017 09:26
vstinner
vstinner requested changes Feb 14, 2017
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.

You need to document the name option in Doc/library/textwrap.rst, don't forget ".. versionchanged:: 3.7" markup.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/textwrap.py Outdated

@vstinner vstinner Feb 14, 2017

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 suggest to make it private and use a different name than len: self._text_width() for example.

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner Feb 14, 2017

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.

Exposing functions mean that you must document them, write unit tests, and that someone (maybe not you) have to maintain them. I don't think that it's worth it. Let's try with something simple, make these functions private.

Sorry, something went wrong.

Uh oh!

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

@fgallaire fgallaire Feb 14, 2017

Copy link
Copy Markdown
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 agree

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/textwrap.py Outdated

@vstinner vstinner Feb 14, 2017

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.

There is not need to repeat cjk here, there is already a generic **kwargs. Same for other functions below.

Sorry, something went wrong.

Uh oh!

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

@fgallaire fgallaire Feb 14, 2017

Copy link
Copy Markdown
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.

This is the same need as width: to be visible so easily usable.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/textwrap.py Outdated

@vstinner vstinner Feb 14, 2017

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.

You can write {'F', 'W'}, it's optimized as a constant frozenset.

Sorry, something went wrong.

Uh oh!

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

@fgallaire fgallaire Feb 14, 2017

Copy link
Copy Markdown
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's really faster than a tuple ?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/textwrap.py Outdated

@vstinner vstinner Feb 14, 2017

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 _ in the name: cjk_wide().

Note sure about the name: is_cjk_wide_char()?

Do we really need to make the function?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/textwrap.py Outdated

@vstinner vstinner Feb 14, 2017

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.

Rename to cfk_width()?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/textwrap.py Outdated

@vstinner vstinner Feb 14, 2017

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 this case. Why do you pass a non-string to the function?

Sorry, something went wrong.

Uh oh!

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

@fgallaire fgallaire Feb 14, 2017

Copy link
Copy Markdown
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.

This is for people who want to do:
from textwrap import cjklen as len
and use cjklen transparently

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/textwrap.py Outdated

@vstinner vstinner Feb 14, 2017

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.

IMHO it's better to make all these new functions private: add _ prefix. Rename to _cjk_slices().

"index": is it a number of character or a width? Maybe rename to width?

Sorry, something went wrong.

Uh oh!

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

@fgallaire fgallaire Feb 14, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
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.

No, I really want this functions to be exported, they are useful

Sorry, something went wrong.

Uh oh!

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

vstinner commented Feb 14, 2017

Copy link
Copy Markdown
Member

By email I proposed a different design. Make existing TextWrapper extensible: add text_width() and truncate() methods. Then add a new TextWrapperCJK which overrides these methods. It would allow to reuse the code for other cases than just CJK.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner requested changes Feb 14, 2017
View reviewed changes
Comment thread Lib/textwrap.py Outdated

@vstinner vstinner Feb 14, 2017

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.

One import per line please.

Sorry, something went wrong.

Uh oh!

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

@fgallaire fgallaire Feb 14, 2017

Copy link
Copy Markdown
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 agree

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/textwrap.py Outdated

@vstinner vstinner Feb 14, 2017

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.

Exposing functions mean that you must document them, write unit tests, and that someone (maybe not you) have to maintain them. I don't think that it's worth it. Let's try with something simple, make these functions private.

Sorry, something went wrong.

Uh oh!

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

vstinner commented Feb 14, 2017

Copy link
Copy Markdown
Member

Your change breaks Python build: Python requires optparse to compile modules like unicodedata, optparse imports textwrap which now always requires unicodedata.

Using two different classes, it would be simpler to only import unicodedata when the TextWrapperCJK class is instanciated, "on demand", and so fix the bootstrap issue.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@fgallaire
fgallaire force-pushed the master branch from 4745076 to 5c4b2af Compare February 14, 2017 10:19
@fgallaire

fgallaire commented Feb 15, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Author

What about a depreciation warning to inform that cjk default will switch to True in Python 3.8 ?
All reactions

Sorry, something went wrong.

Uh oh!

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

fgallaire commented Feb 15, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Author

optparse is deprecated since Python 3.2 so it should not drive this work, but a port to argparse will not solve this problem because of the same cycling dependency.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
fgallaire added 7 commits February 15, 2017 03:32
@fgallaire
Fix TextWrapper positionnal arguments
aa94f26
@fgallaire
Fix one import per line
0264d9d
@fgallaire
Rename self.len() in self._width()
d630821
@fgallaire
Rename CJK functions with _
bfdfb22
@fgallaire
Improve cjk_slices() complexity from O(n^2) to O(n) (Thanks to INADA …
8337ce5 
…Naoki)
@fgallaire
Add Doc for new CJK option and functions
cb9812b
@fgallaire
Fix Python build problems
54de7aa
@fgallaire
fgallaire force-pushed the master branch from 8575fb4 to 54de7aa Compare February 15, 2017 02:44
@codecov

codecov Bot commented Feb 15, 2017

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (master@984eef7). Click here to learn what that means.
The diff coverage is 72.5%.

@@            Coverage Diff            @@
##             master      #89   +/-   ##
=========================================
  Coverage          ?   82.38%           
=========================================
  Files             ?     1428           
  Lines             ?   351193           
  Branches          ?        0           
=========================================
  Hits              ?   289333           
  Misses            ?    61860           
  Partials          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 984eef7...54de7aa. Read the comment docs.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
methane
methane reviewed Feb 15, 2017
View reviewed changes

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

While I agree supporting east_asian_wdith is preferable, I don't like current API.
It's OK for third party library. But for standard library, I prefer more generic API and implementation.

While "practical beats purity", I want glibc's wcwidth for UTF-8 at least. (bonus: option like ambiwidth=double in vim)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/library/textwrap.rst

.. function:: cjk_len(text)

Return the real width of *text* (its len if not a string).

@methane methane Feb 15, 2017

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 like wording real.
How about zero-width space? How about combining character sequence? variation selector?
EMOJI MODIFIER?

Sorry, something went wrong.

Uh oh!

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

@fgallaire fgallaire Feb 15, 2017

Copy link
Copy Markdown
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.

You're probably right. The real purpose is to help to understand that width (cjk_len) and len are different. What do you think about visual instead ?

Sorry, something went wrong.

Uh oh!

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

yan12125 commented Feb 16, 2017

Copy link
Copy Markdown

I want glibc's wcwidth for UTF-8 at least

Disagreed. I guess text editors and terminals are top two applications in textwrap. I known little about text editors. In terminals, things are quite interesting. Different terminals use different versions of EastAsianWidth.txt. Here are some examples:

  • tmux, QTerminal: use wcwidth() by default. In the latest glibc, it's still Unicode 8.0. [1] On Mac, it's reported that wcwidth() is broken [2].
  • VTE (gnome-terminal, xfce4-terminal, etc.): Unicode 9.0 [3]
  • Konsole: Unicode 5.0 [4]

Different Unicode versions can lead to different results for textwrap. Take U+231A (clock emoji) for example. In Unicode 8.0 it's NEUTRAL. Most implementations see it as width=1. In Unicode 9.0 it's changed to WIDE.

In CPython, handling multiple Unicode versions sounds impractical. IMO chasing the latest Unicode 9.0 is a good idea. If a terminal emulator is not compatible with Unicode 9.0, it should be fixed.

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=20313
[2] tmux/tmux#515
[3] https://bugzilla.gnome.org/show_bug.cgi?id=771591
[4] https://github.com/KDE/konsole/blob/master/src/konsole_wcwidth.cpp#L55
All reactions

Sorry, something went wrong.

Uh oh!

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

yan12125 commented Feb 16, 2017

Copy link
Copy Markdown

Also, I would recommend rename cjk_foobar stuffs to unicode_foobar. CJK characters occupy a large portion on Unicode tables but not the whole.
All reactions

Sorry, something went wrong.

Uh oh!

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

methane commented Feb 16, 2017

Copy link
Copy Markdown
Member

@yan12125 sorry, my wording was wrong. (I'm not good at English).
When I said "I want glibc's wcwidth for UTF-8 at least", I meant
"if textwrap supports display width, I think it should be good as wcwidth at least.".
I didn't meant neither "textwrap should use wcwidth" nor "textwrap should implement algorithm
exactly same to wcwidth."

BTW, pull request is not place for discussion like this.
Please go to http://bugs.python.org/issue24665
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
duboviy
duboviy suggested changes Mar 11, 2017
View reviewed changes
Comment thread Lib/textwrap.py
Fullwidth and Wide CJK chars are double-width.
"""
import unicodedata
return unicodedata.east_asian_width(char) in ('F', 'W')

This comment was marked as resolved.

Sign in to view

Sorry, something went wrong.

Uh oh!

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

@fgallaire fgallaire Mar 12, 2017

Copy link
Copy Markdown
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.

Please, @duboviy or @Haypo, could you explain to me why/how a tuple could be slower than a frozenset.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/textwrap.py
def cjk_len(text):
"""Return the real width of text (its len if not a string).
"""
if not isinstance(text, str):

@duboviy duboviy Mar 11, 2017

Copy link
Copy Markdown

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.

Strange case handling, maybe we should expect only string type text argument in this function...

Sorry, something went wrong.

Uh oh!

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

@fgallaire fgallaire Mar 12, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
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.

Again: it's for an handy replacement of the built-in len():
from textwrap import cjk_len as len

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
duboviy
duboviy suggested changes Mar 11, 2017
View reviewed changes
Comment thread Misc/ACKS
Santiago Gala
Yitzchak Gale
Matthew Gallagher
Florent Gallaire

@duboviy duboviy Mar 11, 2017

Copy link
Copy Markdown

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.

Not sure if that makes lots of sense for such a small change (even though such changes are very welcome of course!)

There are commits from loads of people in this repo, but way less entries in this file (adding every contributor would quickly make the file pretty chaotic).

IMO it'd be better to only add yourself to this file when you contribute e.g. a major fix, new feature, etc

Sorry, something went wrong.

Uh oh!

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

@fgallaire fgallaire Mar 12, 2017

Copy link
Copy Markdown
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 strongly disagree, if persons are missing they should be added.

Sorry, something went wrong.

Uh oh!

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

@terryjreedy terryjreedy Apr 27, 2017

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.

Florent definitely belongs in ACKS. We have been fairly liberal in adding people, some for as little as a well-crafted sentence or line of code.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
terryjreedy
terryjreedy reviewed Apr 27, 2017
View reviewed changes
Comment thread Lib/textwrap.py
"""Return True if char is Fullwidth or Wide, False otherwise.
Fullwidth and Wide CJK chars are double-width.
"""
import unicodedata

@terryjreedy terryjreedy Apr 27, 2017

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 think importing unicodedata, when wanted, for each char, is wrong. If not imported at the top, it could be imported as a global in TextWrapper.init when cjk is true. (I am assuming that the convenience functions all instantiate TextWrapper.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
akruis pushed a commit to akruis/cpython that referenced this pull request Sep 9, 2017
Anselm Kruis
Stackless issue python#81 and python#89: Fix the thread and interpret…
dd01c8e 
…er shutdown code

Fix the implementation to work as documented if a thread dies. Now
Stackless kills only tasklets with a nesting level > 0. During interpreter
shutdown stackless additionally kills daemon threads, if they execute Python code or
switch tasklets. This prevents access violations after clearing the thread states.

Add a 1ms sleep for each daemon thread. This way the thread gets a better
chance to run. Thanks to Kristján Valur Jónsson for suggesting this improvement.

Add a test case for a C assertion violation during thread shutdown.
Add some explanatory comments to the shutdown test case.
Thanks to Christian Tismer for the sugestion.

https://bitbucket.org/stackless-dev/stackless/issues/81
https://bitbucket.org/stackless-dev/stackless/issues/89
(grafted from 2cc41427a347a1ebec4eedc3db06a3664e67f798, 1388a2003957,
6140f5aaca2c and edc9b92ec457)
akruis pushed a commit to akruis/cpython that referenced this pull request Sep 9, 2017
Anselm Kruis
merge 3.4-slp (Stackless python#81, python#89)
3c981be
@JulienPalard JulienPalard mentioned this pull request Feb 13, 2018
bpo-24665: Add CJK support in textwrap by default. #5649
Closed
@brettcannon

brettcannon commented Feb 21, 2018

Copy link
Copy Markdown
Member

To try and help move older pull requests forward, we are going through and backfilling 'awaiting' labels on pull requests that are lacking the label. Based on the current reviews, the best we can tell in an automated fashion is that a core developer requested changes to be made to this pull request.

If/when the requested changes have been made, please leave a comment that says, I have made the requested changes; please review again. That will trigger a bot to flag this pull request as ready for a follow-up review.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@brettcannon brettcannon added the awaiting changes label Feb 21, 2018
@methane

methane commented Jul 8, 2018

Copy link
Copy Markdown
Member

I close this because I don't like APIs this PR has and the author seems satisfied by
putting his library on PyPI.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@methane methane closed this Jul 8, 2018
@alvinhochun alvinhochun mentioned this pull request Oct 22, 2021
Wrapping of PO file should take into account the East Asian Width (wide/fullwidth) translate/translate#4452
Closed
jaraco pushed a commit that referenced this pull request Dec 2, 2022
@pyup-bot @Mariatta
Update cherry_picker from 1.0.0 to 1.1.1 (GH-89)
c0393de
jaraco added a commit to jaraco/cpython that referenced this pull request Feb 17, 2023
@jaraco
Build docs without docutils. Closes python#89.
c95a0d8
@gvanrossum gvanrossum mentioned this pull request Aug 22, 2023
heap-use-after-free in _PyFunction_LookupByVersion #108253
Closed
@ngoldbaum ngoldbaum mentioned this pull request Sep 23, 2024
Crash running PyO3 tests with --test-threads=1000 #124375
Closed
This was referenced Feb 11, 2025
segmentfault when pip installing setuptools #129992
Closed
segmentfault when pip installing setuptools #129993
Closed
@fgallaire fgallaire mannequin mentioned this pull request Apr 10, 2022
CJK support for textwrap #68853
Closed
@sergey-miryanov sergey-miryanov mentioned this pull request Dec 18, 2025
Observed memory leak in ssl library: Python 3.14 GC issue #142516
Closed
SonicField added a commit to SonicField/cpython that referenced this pull request Apr 23, 2026
@SonicField
docs: W28 spec — cinder_opcode.h / opcode.h header-guard collision
0e0cea8 
Per supervisor [chat L2626] + L2742 + librarian L2737 (close pythia python#87
python#3 ephemera-gap durably).

Captures empirical W26 push 84 incident (95c9f9b → 1553c14 fix):
- Both cinder_opcode_ids.h + Include/opcode.h use Py_OPCODE_H guard
- Include order silently shadows whichever loads second
- W26 case: BINARY_OP_ADD_INT undefined → attr_probe HIR regression
  caught by W21 golden trip-wire

Three resolution options enumerated:
- (A) rename cinder_opcode_ids.h guard to CINDERX_OPCODE_IDS_H —
  recommended, eliminates collision class
- (B) static_assert in cinder_opcode.h to detect collision at compile
  — hardens detection without fixing
- (C) header-comment only — current state, NOT recommended (chat-
  ephemera proven insufficient)

Related W29 candidate noted: PHX_PRIM_OP_* / PHX_PRIM_UOP_* hard-coded
in builder_emit_c.c lines 3727-3746 with no static_assert binding to
classloader.h authoritative #defines (per pythia python#89 python#3 re-issue).

DEFERRED per supervisor — schedule post-Batch-2 burndown, before any
upstream sync touching Include/opcode.h or classloader.h.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@methane methane methane requested changes
@terryjreedy terryjreedy terryjreedy left review comments
@vstinner vstinner vstinner requested changes
+1 more reviewer
@duboviy duboviy duboviy requested changes
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

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.

9 participants

@fgallaire @the-knights-who-say-ni @vstinner @yan12125 @methane @brettcannon @duboviy @terryjreedy @zware
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.