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

Save previous message for fuzzy matches #316

Merged
merged 1 commit into from
Dec 20, 2022

Conversation

maennchen
Copy link
Member

Should this be the default behavior or hidden behind a flag?

@whatyouhide
Copy link
Contributor

How does it affect what gets written to the PO files?

@maennchen
Copy link
Member Author

@whatyouhide When we fuzzy match on a message, the new message is in the msgid and the old one will be stored as a previous message.

It will look something like this:
https://github.com/elixir-gettext/expo/blob/d3b6377edf24884f290efcaae1f5e346c39e3555/test/expo/po_test.exs#L500

@maennchen
Copy link
Member Author

@whatyouhide It would probably make sense to handle this the same as #315

@maennchen
Copy link
Member Author

maennchen commented Oct 3, 2022

@whatyouhide Do you have an update so that I can finish the PR?

@whatyouhide
Copy link
Contributor

@maennchen sorry for the long wait. I agree, having this behind an option (like :on_obsolete) sounds great. By the way, #| is the "official" GNU Gettext comment for old fuzzy matches, right?

@maennchen
Copy link
Member Author

@whatyouhide I‘ll do that 👍

No, the official way fur fuzzy is the fuzzy flag.

The pipe comment signals the previous mesage.

If I extrat the string Hello World and change it to Hello Worlds, we identify it as fuzzy. We no longer have the initial extracted string though.

With bigger translations, it can be helpful for the translator to know how the message changed so that he can apply the same change to the current translation.

@whatyouhide
Copy link
Contributor

Yeah I’m down for that 👍 My question was: do "previous translations" have an official comment in GNU Gettext?

@maennchen
Copy link
Member Author

@whatyouhide Yes, the #| syntax is official.

See: https://www.gnu.org/software/gettext/manual/html_node/PO-Files.html

white-space
#  translator-comments
#. extracted-comments
#: reference…
#, flag…
#| msgid previous-untranslated-string
msgid untranslated-string
msgstr translated-string

[...]

The previous-untranslated-string is optionally inserted by the msgmerge program, at the same time when it marks a message fuzzy. It helps the translator to see which changes were done by the developers on the untranslated-string.

@whatyouhide
Copy link
Contributor

Ah that's fantastic. Let's go for the option then, and we'll be good to go here.

@maennchen
Copy link
Member Author

@whatyouhide I'll add that in next week 👍

lib/gettext.ex Outdated
@@ -606,6 +606,11 @@ defmodule Gettext do
If `:mark_as_obsolete`, messages are kept and marked as obsolete.
If `:delete`, obsolete messages are deleted. Defaults to `:delete`.

* `:on_fuzzy_match` - controls what happens when fuzzy messages matches are found.
Copy link
Member Author

Choose a reason for hiding this comment

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

@whatyouhide I'm not happy with the naming of this option. Do you have a better idea than this?

Copy link
Contributor

Choose a reason for hiding this comment

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

@maennchen since the behaviours here are not complementary, but rather one a subset of the other, I think we can have store_previous_message_on_fuzzy_match: boolean(). The option name is quite long, but I like that it really conveys what it's doing. Regardless of whether the option is true or false, we'll still override the message and mark it as fuzzy, so the option only controls whether we store the old message too. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

@whatyouhide I agree that a longer more descriptive name is the way to go. I'll adapt accordingly.

Copy link
Member Author

Choose a reason for hiding this comment

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

@whatyouhide Done, better?

@maennchen maennchen marked this pull request as ready for review December 19, 2022 13:24
@coveralls
Copy link

coveralls commented Dec 19, 2022

Pull Request Test Coverage Report for Build f9f5149cf47d6c99001d57308041fe7f890040c6-PR-316

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.07%) to 89.61%

Totals Coverage Status
Change from base Build b720495c9700bfcf87fd149e1e796502ba75f858: 0.07%
Covered Lines: 552
Relevant Lines: 616

💛 - Coveralls

@whatyouhide
Copy link
Contributor

Awesome work, thanks @maennchen! 💟

@whatyouhide whatyouhide merged commit 2d9228c into elixir-gettext:main Dec 20, 2022
@maennchen maennchen deleted the previous_messages branch December 20, 2022 11:11
ravensiris pushed a commit to ravensiris/gettext that referenced this pull request Aug 3, 2023
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.

None yet

3 participants