refactor: replace the pofile dependency with pofile-ts
#2379
Conversation
|
@swernerx is attempting to deploy a commit to the Crowdin Team on Vercel. A member of the Team first needs to authorize it. |
swernerx
changed the title
pofile dependency with pofile-tspofile dependency with pofile-ts
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Interesting... I was not aware of any other work on this... I assume it's related but mine might be even more up-to-date also covering a series of new test cases. |
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (53.12%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2379 +/- ##
==========================================
- Coverage 77.05% 76.56% -0.49%
==========================================
Files 84 100 +16
Lines 2157 2748 +591
Branches 555 718 +163
==========================================
+ Hits 1662 2104 +442
- Misses 382 511 +129
- Partials 113 133 +20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@swernerx The background of this package is that I initially made the update for the same reasons you mentioned - to modernize it, fix the TypeScript typing issues, and generally bring it up to date. My intention, however, was to transfer it under the Lingui organization. This package is quite important for Lingui, and ideally it should be maintained by the community or the organization rather than by a random guy from the internet (in this case, me). I faced some resistance from the Lingui maintainers on moving it into the organization, which is why the package hasn’t been published yet. By the way, the original implementation has several known issues in Lingui repo (#2235 #2098). I still believe there’s real value in bringing this package into the organization and addressing everything together as a single project. Another option would be completely change implementation for the https://www.npmjs.com/package/gettext-parser not sure how close this to the gettext spec though |
swernerx
force-pushed
the
sw/migrate-to-pofile-ts
branch
from
08518d0 to
f688222
Compare
|
@timofei-iatsenko Thanks for the context! I looked at the previous approach, but I'd argue against integrating PO parsing directly into Lingui for a few reasons:
I think keeping PO parsing as a separate, well-maintained library is the more pragmatic path forward. |
swernerx
force-pushed
the
sw/migrate-to-pofile-ts
branch
from
f688222 to
5da2832
Compare
|
@swernerx it's not about integrating pofile into lingui, it's about hosting the package under the lingui org. So the package would be still separate, installable, but would be at full control of the lingui organization and with more trust in it from the consumers. The problem with packages implemented by some random guy, is that, people usually don't realize how much work it would be for them after releasing something like that and burnout quite quickly leaving the package abandoned. |
|
Fair point about maintainer burnout—I've been doing open source for years and know exactly what you mean. That said, I'd suggest a step-by-step approach here:
Let's not let perfect be the enemy of good. This PR improves the status quo today. Organizational decisions about package ownership can happen separately. |
|
@swernerx meanwhile maybe you can work on these problems in your fork: Multiline formatting issue: #2235 Here i would suggest to have a wrap or fold option similar to gettext-parser's https://www.npmjs.com/package/gettext-parser#:~:text=with%20possible%20values-,foldLength,-is%20the%20length
This later might be exposed as the formatter parameter. Also would be nice to dig into original source and specs to understand why msgid ""
"Something\n"
"or something else"This change might be a disruptive a bit, i would not consider it's breaking because, files generated would be still consumable and result should not be changed, but changes here will definitely produce a diff in consumers codebases. PO translator comments must begin with # and space #2098 This should be easy to fix. Will also produce diff but not breaking. |
Indeed my fork had the same issue. I fixed that and also implemented two new options to tweak it as needed for different usage models. Fixes: #2379 (hopefully) ;) |
| * When `false`, uses GNU gettext's traditional format with an empty first line: | ||
| * ```po | ||
| * msgid "" | ||
| * "First line\n" | ||
| * "Second line" | ||
| * ``` |
There was a problem hiding this comment.
Do you have any links to read about this traditional format? I'm wondering was it a indeed a traditional and made intentionally by author of the pofile or it was done unintentionally and we keeping them unnecessarily
There was a problem hiding this comment.
The empty string convention for multiline entries in PO files is documented in the GNU gettext manual, section "The Format of PO Files":
Link: https://www.gnu.org/software/gettext/manual/html_node/PO-Files.html
The format uses C-style string literal concatenation — adjacent quoted strings are automatically joined. The empty string "" on the first line is a stylistic convention that:
- Improves alignment of subsequent lines
- Enhances readability
- Has no semantic effect (empty string + rest = rest)
The underlying mechanism comes from the C specification (ISO C99 Section 6.4.5 "String literals" and 5.1.1.2 "Translation phases", Phase 6).
There was a problem hiding this comment.
For reference the exact example with detailed description is here
https://www.gnu.org/software/gettext/manual/html_node/More-Details.html#Further-details-on-the-PO-file-format
What i'm thinking, is that if this pattern is stated in the official docs - this should be a default, not the "compact" style, wdyt?
I would also put a link to this doc into the flag description.
There was a problem hiding this comment.
I was a bit undecided here. My thinking was mostly shaped by the Crowdin use case and other automated tooling I’ve worked with recently – for those tools the concrete PO formatting is effectively irrelevant. Since Crowdin is one of the bigger players, I considered its behavior a reasonable “de-facto” default. In our setup people almost never open the PO files manually and 99.9% of translations happen fully automatically, so readability of multiline entries hasn’t really been a concern so far.
That said, if the GNU-style empty-string convention is explicitly recommended in the official docs and improves readability for anyone who does look at the files, I’m perfectly fine with using that as the default and treating the compact form as an alternative style.
There was a problem hiding this comment.
Since Crowdin is one of the bigger players, I considered its behavior a reasonable “de-facto” default
I would consider a deafult example written in the gettext official docs.
In our setup people almost never open the PO files manually and 99.9% of translations happen fully automatically, so readability of multiline entries hasn’t really been a concern so far.
Same here.
|
@swernerx both po formatters has this ugly piece of code: // accessing private property
;(po as any).headerOrder = Object.keys(po.headers)The |
Fixed |
|
@swernerx now you need to update and fix conflicts from the main branch. |
Tests fail without running yarn release:build first, as @lingui/cli and other packages need to be compiled before they can be resolved.
- Replace pofile dependency with pofile-ts ^2.2.0 - Use exported types (Item, Headers) instead of manual definitions - Use fs.writeFile + po.toString() instead of removed po.save() - Add pofile-ts to Jest transformIgnorePatterns for ESM support - Add .lingui/ to .gitignore (test cache artifacts) pofile-ts is a modernized fork with TypeScript support and zero dependencies (pure parsing/serialization, no filesystem access).
BREAKING CHANGE: pofile-ts v3.0.0 uses a functional API instead of classes Migration: - PO.parse() → parsePo() - po.toString() → stringifyPo(po) - new PO() → createPoFile() - new PO.Item() → createItem() - PO class → PoFile interface - Item class → PoItem interface
Upgrade pofile-ts to v3.1.0 and expose new serialization options: - foldLength: Maximum line width before folding (default: 80, 0 = disable) - compactMultiline: Compact format for multiline strings (default: true) These options enable better compatibility with translation platforms like Crowdin that may have issues with empty first lines in multiline strings. Refs: lingui#2235
headerOrder is a public property on PoFile, no cast needed.
swernerx
force-pushed
the
sw/migrate-to-pofile-ts
branch
from
77290d4 to
65860a1
Compare
Done |
|
@swernerx it's failing on the tests now. I'm wondering may be you can also look at the performance side of that pofile parsing? We had a discussion #2320 (comment) some time ago which you might be interested to follow. TL; DR; When you have a quite big catalog parsing is taking a significant time. I'm not sure is it something that could be improved a lot, but at least you could give it a try. |
|
@swernerx CI still failing |
|
@swernerx Is there anything to add or we can review and move this forward? |
2 participants
Replaces the
pofiledependency withpofile-ts, a modernized fork with full TypeScript support.Why this change?
@types/package neededsave()method instead of callbackspofilelast release was December 2022Affected packages:
@lingui/format-po@lingui/format-po-gettext@lingui/cliCode improvements:
Item,Headers) directly frompofile-tspo.save()to Promise-based APIOther:
pofile-tsto JesttransformIgnorePatternsfor ESM support.lingui/to.gitignore(test cache artifacts)yarn release:buildstep in CONTRIBUTING.mdTypes of changes
Checklist