Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-130703: Implement wrapping to width for msgids#130705

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
StanFromIreland wants to merge24 commits intopython:main
base:main
Choose a base branch
Loading
fromStanFromIreland:respect-width

Conversation

StanFromIreland
Copy link
Contributor

@StanFromIrelandStanFromIreland commentedFeb 28, 2025
edited by bedevere-appbot
Loading

tomasr8 reacted with thumbs up emoji
@StanFromIreland
Copy link
ContributorAuthor

Requesting@tomasr8@serhiy-storchaka :-)

Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This does not work.

  • It can break escape sequences.
  • The normalized message can already be multiline. Splitting it again will produce too short lines and even empty lines.

@StanFromIrelandStanFromIreland marked this pull request as draftFebruary 28, 2025 20:10
@StanFromIreland
Copy link
ContributorAuthor

I need to update normalize to wrap respecting words

Copy link
Member

@picnixzpicnixz left a comment
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Can't you usetextwrap.wrap for wrapping? It's not perfect but it ought to detect do most of the job?

@tomasr8
Copy link
Member

tomasr8 commentedFeb 28, 2025
edited
Loading

I'm afraid textwrap won't always work. I suggest adding the wrapping logic to the normalize function. pybabel does it in a similar way, you can have a look at their implementation:https://github.com/python-babel/babel/blob/master/babel/messages/pofile.py#L464

StanFromIreland reacted with thumbs up emoji

@StanFromIreland
Copy link
ContributorAuthor

Implemented pybabels method.

Copy link
Member

@picnixzpicnixz left a comment
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'm not really the best for reviewing this but I can review the implementation. Please, don't just apply my suggestions as is and decide which one is the best.

StanFromIreland reacted with heart emoji
@StanFromIrelandStanFromIreland marked this pull request as ready for reviewMarch 1, 2025 10:20
StanFromIrelandand others added2 commitsMarch 1, 2025 11:03
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Copy link
Member

@picnixzpicnixz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'm not sure that the regex approach is correct. It would gobble up consecutive spaces right?

@tomasr8
Copy link
Member

I really recommend creating a dummy file with some gettext calls and comparing the differences between pygettext, xgettext and babel. There are some differences that should be considered. Here's two I noticed:

  • The header is not wrapped but both xgettext and babel do wrap it.
  • This file:
_('foos')

ran with--width=3 produces this output:

msgid """""foos"msgstr ""

while xgettext and babel give me this (i.e. they don't insert two extra"" when the line does not get wrapped):

msgid"foos"msgstr""

@StanFromIreland
Copy link
ContributorAuthor

StanFromIreland commentedMar 2, 2025
edited
Loading

As for the header, this will conflict with my implementation of--omit-header, could that get merged first (or vice versa)?

@StanFromIreland
Copy link
ContributorAuthor

StanFromIreland commentedMar 2, 2025
edited
Loading

Test fail unrelated.

Wrapping header will require a separate function like so:

Subject: [PATCH] Wrap header---Index: Tools/i18n/pygettext.pyIDEA additional info:Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP<+>UTF-8===================================================================diff --git a/Tools/i18n/pygettext.py b/Tools/i18n/pygettext.py--- a/Tools/i18n/pygettext.py(revision 8d03cbf141068c4ac9812a967a4c9f5942e22d75)+++ b/Tools/i18n/pygettext.py(date 1740913002600)@@ -589,12 +589,36 @@     def _is_string_const(self, node):         return isinstance(node, ast.Constant) and isinstance(node.value, str)++def _wrap_header(s, options):+    lines = []+    for line in s.splitlines():+        if len(line) > options.width and ' ' in line:+            words = _space_splitter(line)+            words.reverse()+            buf = []+            size = 0+            while words:+                word = words.pop()+                if size + len(word) <= options.width:+                    buf.append(word)+                    size += len(word)+                else:+                    lines.append(''.join(buf))+                    buf = [word]+                    size = len(word)+            lines.append(''.join(buf))+        else:+            lines.append(line)+    return "\n".join(lines) + "\n"++ def write_pot_file(messages, options, fp):     timestamp = time.strftime('%Y-%m-%d %H:%M%z')     encoding = fp.encoding if fp.encoding else 'UTF-8'-    print(pot_header % {'time': timestamp, 'version': __version__,+    print(_wrap_header(pot_header % {'time': timestamp, 'version': __version__,                         'charset': encoding,-                        'encoding': '8bit'}, file=fp)+                        'encoding': '8bit'}, options), file=fp)      # Sort locations within each message by filename and lineno     sorted_keys = [

@picnixzpicnixz removed their request for reviewMarch 2, 2025 17:18
Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It looks almost ready now. Please add more tests for cases. It may be convenient to use the same string with different widths.

Add tests for the cases whenlen(escaped_line) + len(prefix) + 3 equals towidth and when it equals towidth.

Add tests for the cases whennew_size + 2 equals towidth and when it equals towidth + 1.

Add tests for too long first word (new_size + 2 >widthandbuf` is empty) and for too long last word.

Add tests for whitespaces other than' ' and'\n' (e.g. for'\t' and'\r'), for non-ASCII line separators and whitespaces. Test for different escaping mode.

Do not add a separate method for every case. Group assertions for similar cases in one method.

StanFromIreland reacted with thumbs up emoji
@StanFromIreland
Copy link
ContributorAuthor

Friendly ping@serhiy-storchaka :-)

@serhiy-storchaka
Copy link
Member

Sorry, the tests still do not satisfy me. I am going to play with them myself, and then propose my variant.

StanFromIreland reacted with thumbs up emoji

@picnixzpicnixz removed their request for reviewMarch 23, 2025 12:26
@serhiy-storchakaserhiy-storchaka self-assigned thisApr 9, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@picnixzpicnixzpicnixz left review comments

@savannahostrowskisavannahostrowskiAwaiting requested review from savannahostrowski

@serhiy-storchakaserhiy-storchakaAwaiting requested review from serhiy-storchaka

@tomasr8tomasr8Awaiting requested review from tomasr8

Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@StanFromIreland@tomasr8@serhiy-storchaka@picnixz

[8]ページ先頭

©2009-2025 Movatter.jp