msginit: Replace charset in Content-type with charset in locale #113
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`getext` is generated with `charset=CHARSET`. Translation as is will result in an error because `CHARSET` is an invalid value. Changed so that `charset=CHARSET` is replaced by the locale's charset.
kou
reviewed
Jan 22, 2025
lib/gettext/po_parser.rb
Outdated
lib/gettext/tools/msginit.rb
Outdated
| @@ -325,6 +332,14 @@ def replace_plural_forms | |||
| end | |||
| end | |||
|
|
|||
| CONTENT_TYPE_CHARSET = /(Content-Type: .+ charset=)CHARSET/ | |||
There was a problem hiding this comment.
Suggested change
| CONTENT_TYPE_CHARSET = /(Content-Type: .+ charset=)CHARSET/ | |
| CONTENT_TYPE_CHARSET = /^(Content-Type:.+ charset=)CHARSET/ |
lib/gettext/tools/msginit.rb
Outdated
Comment on lines
338
to
340
| if CONTENT_TYPE_CHARSET =~ @entry | ||
| @entry = @entry.gsub(CONTENT_TYPE_CHARSET, "\\1#{@charset}") | ||
| end |
There was a problem hiding this comment.
Suggested change
| if CONTENT_TYPE_CHARSET =~ @entry | |
| @entry = @entry.gsub(CONTENT_TYPE_CHARSET, "\\1#{@charset}") | |
| end | |
| @entry = @entry.gsub(CONTENT_TYPE_CHARSET, "\\1#{@charset}") |
test/po/ja/_.po
Outdated
test/tools/test_msginit.rb
Outdated
| @@ -196,8 +198,8 @@ def test_specify_option | |||
| end | |||
|
|
|||
| class TestLocale < self | |||
| def run_msginit(locale) | |||
| create_pot_file("test.pot") | |||
| def run_msginit(locale, charset = "UTF-8") | |||
There was a problem hiding this comment.
Suggested change
| def run_msginit(locale, charset = "UTF-8") | |
| def run_msginit(locale, charset=nil) |
test/tools/test_msginit.rb
Outdated
Comment on lines
230
to
249
| def test_language_charset_with_replace_content_type | ||
| locale = "en" | ||
| assert_equal(po_header(locale, locale), | ||
| run_msginit(locale, "CHARSET")) | ||
| end | ||
|
|
||
| def test_language_region_with_replace_content_type | ||
| locale = "en_US" | ||
| language = "en" | ||
| assert_equal(po_header(locale, language), | ||
| run_msginit(locale, "CHARSET")) | ||
| end | ||
|
|
||
| def test_language_region_charset_with_replace_content_type | ||
| locale = "en_US" | ||
| language = "en" | ||
| charset = "UTF-8" | ||
| assert_equal(po_header(locale, language), | ||
| run_msginit("#{locale}.#{charset}", "CHARSET")) | ||
| end |
There was a problem hiding this comment.
Why do we need them?
It seems that existing tests cover these cases.
There was a problem hiding this comment.
The test was to check that charset is replaced as expected, even when --locale is specified.
This is because the value of language_tag changes with or without its option.
gettext/lib/gettext/tools/msginit.rb
Lines 169 to 173 in 17ad4cd
There was a problem hiding this comment.
OK.
Let's clarify it by test name and variable name:
-test_XXX_with_replace_content_type
+test_XXX_with_template_charset-run_msginit("XXX", "CHARSET")
+template_charset = "CHARSET"
+run_msginit("XXX", template_charset)
test/tools/test_msginit.rb
Outdated
|
|
||
| class TestCurrentCharset < self | ||
| def run_msginit(charset) | ||
| create_pot_file("test.pot", :charset => charset) |
There was a problem hiding this comment.
Suggested change
| create_pot_file("test.pot", :charset => charset) | |
| create_pot_file("test.pot", charset: charset) |
test/tools/test_msginit.rb
Outdated
| end | ||
|
|
||
| class TestCurrentCharset < self | ||
| def run_msginit(charset) |
There was a problem hiding this comment.
Suggested change
| def run_msginit(charset) | |
| def run_msginit(pot_charset) |
test/tools/test_msginit.rb
Outdated
Comment on lines
265
to
266
| assert_equal(po_header(:charset => "UTF-8"), | ||
| run_msginit("CHARSET")) |
There was a problem hiding this comment.
Suggested change
| assert_equal(po_header(:charset => "UTF-8"), | |
| run_msginit("CHARSET")) | |
| assert_equal(po_header(charset: "UTF-8"), | |
| run_msginit("CHARSET")) |
test/tools/test_msginit.rb
Outdated
Comment on lines
270
to
271
| assert_equal(po_header(:charset => "ASCII"), | ||
| run_msginit("ASCII")) |
There was a problem hiding this comment.
Suggested change
| assert_equal(po_header(:charset => "ASCII"), | |
| run_msginit("ASCII")) | |
| assert_equal(po_header(charset: "ASCII"), | |
| run_msginit("ASCII")) |
kou
reviewed
Jan 23, 2025
test/tools/test_msginit.rb
Outdated
| super(current_locale, current_language, options) | ||
| end | ||
|
|
||
| def test_change |
There was a problem hiding this comment.
Suggested change
| def test_change | |
| def test_template_charset |
test/tools/test_msginit.rb
Outdated
| run_msginit("CHARSET")) | ||
| end | ||
|
|
||
| def test_not_change |
There was a problem hiding this comment.
Suggested change
| def test_not_change | |
| def test_not_template_charset |
test/tools/test_msginit.rb
Outdated
| end | ||
|
|
||
| def test_change | ||
| assert_equal(po_header(charset: "UTF-8"), |
There was a problem hiding this comment.
Suggested change
| assert_equal(po_header(charset: "UTF-8"), | |
| assert_equal(po_header(charset: Locale.current.charset), |
We may want to define current_charset like other current_* methods.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
getextis generated withcharset=CHARSET.Translation as is will result in an error because
CHARSETis an invalid value.Changed so that
charset=CHARSETis replaced by the locale's charset.