Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added test for the Khmer language #4253

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

KaiSforza
Copy link
Contributor

@KaiSforza KaiSforza commented Feb 26, 2024

Description

It looks like this was an issue with the _width_table.py file being out of date in some way.

Closes #4235.

Checklist - did you ...

  • Add an entry in CHANGES.md if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

@KaiSforza
Copy link
Contributor Author

@rtb-zla-karma I hope this fixes your issue! Let me know if there's anything else to test here!

(I apologize for the clumsy copy/paste I added in there, I just needed to get a line that was definitely over the limit.)

Copy link

github-actions bot commented Feb 26, 2024

diff-shades results comparing this PR (46e0a24) to main (8990023). The full diff is available in the logs under the "Generate HTML diff report" step.

╭─────────────────────── Summary ────────────────────────╮
│ 1 projects & 2 files changed / 6 changes [+4/-2]       │
│                                                        │
│ ... out of 2 537 182 lines, 12 198 files & 22 projects │
╰────────────────────────────────────────────────────────╯

Differences found.

What is this? | Workflow run | diff-shades documentation

It looks like this was an issue with the `_width_table.py`
file being out of date in some way.

Closes psf#4235.
@rtb-zla-karma
Copy link

Hi @KaiSforza .

It works for me in my project. black with your changes doesn't report anything where the official/released does.

@cobaltt7
Copy link
Contributor

This would have to go in the preview style since it changes existing formatting. Not sure the best way to go about it though, since it looks like it'd be a lot of duplicated code. It might be better to just remember to rerun the script before each January release

@KaiSforza
Copy link
Contributor Author

KaiSforza commented Feb 27, 2024

@RedGuy12 I'll see what I can do. Thanks for the input.

Also, this is a bit of a weird bug, since in #3445 it seems like the _width_table.py file was actually generated with a different script than was committed. Originally the script had this if statement:

if width == 1:
    continue

but the final committed version had a different statement:

if width <= 1:
    continue

This is what is really causing the issue. If I hop into the squashed commit (ef6e079) and run the make_width_table.py script I get the same exact diff as this commit for the width table.

$ git status        
HEAD detached at ef6e079
nothing to commit, working tree clean
$ python ./scripts/make_width_table.py
$ git diff --stat
 src/black/_width_table.py | 364 +---------------------------------------------
 1 file changed, 7 insertions(+), 357 deletions(-)
$ git show --oneline --stat kai/uniwidth
46e0a24 (kaictl/kai/uniwidth, kai/uniwidth) Added test for the Khmer language
 CHANGES.md                                         |   2 +
 src/black/_width_table.py                          | 364 +--------------------
 .../preview_long_strings__east_asian_width.py      |  31 ++
 3 files changed, 40 insertions(+), 357 deletions(-)

To me, this kind of says "it's a bug" and points to not putting it in Preview. From what I can see based on the tests, this shouldn't change anything but a few language strings that are already causing issues in black right now.

(check the commits in #3445 and note that the final commit doesn't change the _width_table.py file, but does change the make_width_table.py)

@KaiSforza
Copy link
Contributor Author

I'm drafting this to do a full rewrite of the unicode character handling. Will probably just push into this same branch to fix it.

@cobaltt7
Copy link
Contributor

To me, this kind of says "it's a bug" and points to not putting it in Preview. From what I can see based on the tests, this shouldn't change anything but a few language strings that are already causing issues in black right now.

Unfortunately, Black promises that formatting generally won't change throughout the calendar year, and this change wouldn't be an exception. See the Stability Policy for more details.

@KaiSforza
Copy link
Contributor Author

That's fine, I'm honestly not sure what the best solution is for languages with ambiguous or non-standardized typesetting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Black joins lines when it shouldn't - short width characters.
3 participants