-
-
Notifications
You must be signed in to change notification settings - Fork 2
v0.5.5 #6
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
base: main
Are you sure you want to change the base?
v0.5.5 #6
Conversation
…t files is used Exactly what it says on the tin.
freakboy3742
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One code style tweak, and one request for clarification - it’s entirely possible you’re correct in what you’ve done, but I’m not sufficiently familiar with msgcat in practice to be confident in that.
|
Um... what's the tweak? Forgot to finish review? |
freakboy3742
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm… not sure what happened there - I must have neglected to confirm the comment I wrote.
The two comments were:
- Using long form flags - using
—sort-by-fileinstead of-F - Can you confirm why this is the right approach? I’m not an expert on msgcat (and I can’t confirm anything manually right now), but
—use-firstsuggests merging keys, which seems preferable to duplication
|
@freakboy3742 Quite the opposite. Merge everything except keys -- but that also means that the "header" is being merged into something like Which is problematic. It's keeping both versions. CONCLUSION hold this PR. just found this issue |
|
That said, xgettext is the canoncial way to merge pots, so I'm trying that. |
|
These concerns should now be resolved. |
|
DO NOT MERGE. It changes the creation date of the POT unnessacraily. |
|
There. This works now. This is what's called Yak Shaving I guess... just to have all the source places merged together, I removed an option from msgcat and realized xgettext is the canoncial way (https://www.gnu.org/software/gettext/manual/html_node/msgcat-Invocation.html, "To concatenate POT files, better use xgettext, not msgcat, because msgcat would choke on the undefined charsets in the specified POT files.") and can handle this better... |
|
Again, as mentioned on the other thread, I really apologize for shaving all those yaks. |
|
Hmm... in the bugfix I used the msgstr number to determine whether the singular form or the plural form needs to get filled in... it's only a good heuristic for some languages, so I documented that and if it's not English I mark the auto-filled plurals as fuzzy. See the changelog/readme for more details. (FYI i patched to fill msgstr with msgids automatcially on the source language po file and clear all the other POs after initial msginit so the bug listed on the README is finally resolved, but here comes plural handling etc etc) |
johnzhou721
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure... the diff is still very large and rewraps a bunch of chinese simplified strings on the beeware.github.io PR even when using ubuntu 24.04... I'm trying to see if this is the trouble.
|
latest commit has a logic error, I will refactor by extracting clearing entry into seperate functions and clearing entry when fill translation entry is fuzzy, this is a note to self. |
|
Hey, John. I'm taking over the final review on this PR. I may be asking for clarification on your changes in this process. My first question is, do you consider this ready for a final review? You repeatedly requested throughout the process that we hold off on merging it, so I want to ensure it's actually ready at this point before moving forward. |
|
@kattni Yes, please! It's ready for final review -- I've made a lot of random changes here and there just to completely fixup this plugin so we hopefully don't need another update again. If you need me to reexplain anything, let me know, as I was quite vague in communication when I started this PR. I apoglogize for the noise. |
|
@johnzhou721 I would appreciate it if you can explain to me how you're testing these changes. I'd like to test it before we get into explanations. |
|
A lot of the testing happens at the beeware.github.io PR where lots of those changes are relevant and applied.
You can see that the new POT at https://github.com/beeware/beeware.github.io/pull/684/files#diff-c8f80bf8f257ddef4811618539fadecda3407d93671f14c95b81e3a161dc2c1c is sorted properly. There's lots of diffs in that file though because of the next change quoted below, however that does make the POT context formatting consistent with the PO files.
Merging of different context is demonstrated at https://github.com/beeware/beeware.github.io/pull/684/files#diff-c8f80bf8f257ddef4811618539fadecda3407d93671f14c95b81e3a161dc2c1cR5102-R5103 -- --use-first of msgcat seems to just use the first context of those strings; however if we remove that flag, the headers are different and the header msgstr will be a complete mess.
Use Now lektor build and the POT will be showing that Project-Id-Version is properly filled in.
OK I've made a mistake in the listing here. Ignore the sentence I've striked through here. pgettext is used for member badges. If the preview of the jinja i18n translation PR renders Katie's "badge" at /zh_CN/about/team/
I'll come up with tests for these 2 a bit later
See preview for this. The missing strings are described at beeware.github.io#689 and they should be there. |
Use
Use So now rerun lektor build and we find that the English PO file automatically populates the new string for Projects -- this doesn't happen in the old version of the plugin, the new Projects string will just stay blank: Now we translate the string in the French version (ce n'est pas une traduction, though) and save the file: Now, we introduce a pluralized version of the string -- we do this in the navigation bar in layout.html: Notice now the English translation is automatically filled correctly, showing that the code for autofilling new source-language translations handles plurals properly: If the same test is performed on French with the source language, the new Projects string will not be automatically filled due to the lack in our ability to parse Plural-Forms to figure out what msgstr index to fill with msgid source and what to fill with plural msgid source. |
|
As noted here, we're putting a pin in this for now, and will pick it up again if needed. |
Exactly what it says on the tin.
Fixes all bugs I can find with this plugin; future updates after this version should be rare.
Changes
Revision July 22
xgettextis used to merge POT files instead ofmsgcat, providing a better header and merging of same strings from different sources. This is used to ensure that all context will be kept; removing —use-first from msgcat simply causes header trouble.Project-Id-Version. This is mostly for my local work — I’ve filled the project id versions manually into the existing POs — but have the good effect of adding a license header (Same license as BeeWare) on top of newly generated POs; I have not went in and added these would’ve-been-generated headers on the existing PO files, though.PR Checklist: