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.

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

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

Conversation

StanFromIreland
Copy link
Contributor

@StanFromIreland StanFromIreland commented Feb 27, 2025

@tomasr8 tomasr8 self-requested a review February 27, 2025 19:20
@StanFromIreland
Copy link
Contributor Author

Also @serhiy-storchaka

@tomasr8

This comment was marked as off-topic.

@StanFromIreland
Copy link
Contributor Author

Anything else left to do here @tomasr8 ?

@StanFromIreland StanFromIreland requested a review from tomasr8 March 9, 2025 22:11
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
Contributor Author

Little reminder @serhiy-storchaka

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
Contributor 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
Contributor 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
Contributor Author

Friendly ping @serhiy-storchaka :-)

Co-authored-by: Stan Ulbrych <89152624+StanFromIreland@users.noreply.github.com>
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.

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

Successfully merging this pull request may close these issues.

3 participants