Skip to content

gh-130647: Add --omit-header option to pygettext #130650

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

gh-130647: Add --omit-header option to pygettext #130650

wants to merge 8 commits into from

Conversation

StanFromIreland
Copy link
Member

@StanFromIreland StanFromIreland commented Feb 27, 2025 •
edited by bedevere-app bot
Loading

@bedevere-app bedevere-app bot mentioned this pull request Feb 27, 2025
Open
@tomasr8 tomasr8 self-requested a review February 27, 2025 19:20
@StanFromIreland
Copy link
Member Author

Also @serhiy-storchaka

@tomasr8

This comment was marked as off-topic.

Sign in to view
@StanFromIreland
Copy link
Member Author

Anything else left to do here @tomasr8 ?

tomasr8
tomasr8 reviewed Mar 2, 2025
View reviewed changes
@StanFromIreland StanFromIreland requested a review from tomasr8 March 9, 2025 22:11
tomasr8
tomasr8 approved these changes Mar 10, 2025
View reviewed changes
Copy link
Member

@tomasr8 tomasr8 left a comment

Choose a reason for hiding this comment

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

LGTM

@StanFromIreland
Copy link
Member Author

Little reminder @serhiy-storchaka

serhiy-storchaka
serhiy-storchaka reviewed Mar 14, 2025
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Please use non-ASCII strings and non-UTF-8 source encoding for tests. The encoding should also be different from Latin1. Maybe iso-8859-15 with € in a string. Use both literal € and \u20ac (in different strings, so the error will be easier to identify).

@StanFromIreland
Copy link
Member Author

Please use non-ASCII strings and non-UTF-8 source encoding for tests.

Might I ask why? The implementation does effect anything beyond the header, it is literally a single if statement. The file is generated so we can verify whitespace, at Tomas's request.

@serhiy-storchaka
Copy link
Member

Why to add this feature at first place? If it is for tests, we should ensure that it works with non-ASCII strings. Otherwise this option will be useless.

We need also tests with non-ASCII strings for gettext and pygettext, but this is a different issue.

@StanFromIreland
Copy link
Member Author

Why to add this feature at first place?

See the linked issue, this was discussed before.

we should ensure that it works with non-ASCII strings.

This feature just removes the POT header.

@StanFromIreland
Copy link
Member Author

Friendly ping @serhiy-storchaka :-)

@serhiy-storchaka @StanFromIreland
Update Tools/i18n/pygettext.py
3b33dd9 
Co-authored-by: Stan Ulbrych <89152624+StanFromIreland@users.noreply.github.com>
serhiy-storchaka
serhiy-storchaka approved these changes May 1, 2025
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

The problem with this feature is that we are losing information about the .po file encoding, which is locale depending. Therefore, files generated on different computers may be incompatible. There is an issue with using the default file encoding in general, because it may be not compatible with the source file encoding (msgids and comments) and filesystem encoding (file names).

We can probably just ignore this for now. And then somehow solve all the encoding problems at once.

But adding a feature that contains an inherent flaw leaves a bitter aftertaste.

@StanFromIreland

This comment was marked as duplicate.

Sign in to view
@StanFromIreland
Copy link
Member Author

Friendly ping @serhiy-storchaka / @tomasr8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@tomasr8 tomasr8 tomasr8 approved these changes

Assignees
No one assigned
Labels
awaiting merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@StanFromIreland @tomasr8 @serhiy-storchaka