-
Notifications
You must be signed in to change notification settings - Fork 393
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
feat: Hash based id #1440
feat: Hash based id #1440
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
9faba10
to
cdda707
Compare
size-limit report 📦
|
cdda707
to
546d172
Compare
1cf60a5
to
4ae8cb5
Compare
4ae8cb5
to
6f3d408
Compare
6f3d408
to
4fdc7ee
Compare
Codecov ReportBase: 72.27% // Head: 73.89% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## next #1440 +/- ##
==========================================
+ Coverage 72.27% 73.89% +1.62%
==========================================
Files 74 75 +1
Lines 1897 1923 +26
Branches 510 513 +3
==========================================
+ Hits 1371 1421 +50
+ Misses 422 383 -39
- Partials 104 119 +15
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Thank you for leading this hard work @thekip this is a top notch long time awaiting feature :) About the hash, would sha256 be a bit more time consuming than sha1 ? |
I have been thinking about what hash function to use for a while. The criteria were - short / with low collision rate / non cryptographical / implemented in Rust I considered Related SO topics: But i couldn't choose and just took what formatjs is using as default, assuming they choose it by the reason. I think for regular catalogs with few thousands of messages, the difference in speed between sha256 or sha1 would be negligible |
That what it seems indeed https://github.com/hex7c0/nodejs-hash-performance 👍 ~same speed, less collision |
@Martin005 @andrii-bodnar guys i want a feedback on these changes before I start porting it to the Rust. Updating test suite is extremely exhausting, so i don't want to do it more than one time. |
@thekip currently I have that one suggestion about the new package also, I'm worrying about this PO flag - it looks like the PO Gettext specification has already a predefined list of allowed flags, isn't it? Or it allows any other flags also? |
@andrii-bodnar new package is not related to Rust macro, so i can start porting.
Yes, there is a predefined list of flags which native gettext tools understand. But specification says more generously: #, flag… And also they say
Means it's designed to affect message interpretation somehow, unlike to other comments which could be safely omitted. |
34362e0
to
4ffaa90
Compare
@thekip We have agreed with @andrii-bodnar that it would be best to postpone the flattening in extraction (drafted in #1431) for now. |
@Martin005 actually to fix issue with flattening we can make #. js-lingui-id: <hash>
msgctxt "..."
msgid "..."
msgstr "" |
Guys, everything related to docs, just do it yourself. I'm just not able to do it all by myself. |
@thekip please just resolve the current suggestions and then we'll take care of its polishing |
Co-authored-by: Martin Chrástek <chrastek12@gmail.com> Apply suggestions from code review Co-authored-by: Andrii Bodnar <29282228+andrii-bodnar@users.noreply.github.com> Update website/docs/ref/macro.md Co-authored-by: Martin Chrástek <chrastek12@gmail.com> Update website/docs/ref/macro.md Co-authored-by: Martin Chrástek <chrastek12@gmail.com> Update website/docs/ref/macro.md Co-authored-by: Martin Chrástek <chrastek12@gmail.com> Update website/docs/ref/macro.md Co-authored-by: Martin Chrástek <chrastek12@gmail.com> Update website/docs/ref/macro.md Co-authored-by: Martin Chrástek <chrastek12@gmail.com> Update website/docs/ref/macro.md Co-authored-by: Martin Chrástek <chrastek12@gmail.com> Update website/docs/ref/macro.md Co-authored-by: Martin Chrástek <chrastek12@gmail.com>
4279eed
to
c0249d3
Compare
Co-authored-by: Martin Chrástek <chrastek12@gmail.com>
Implementation for: #1360
For easier reviewing, i split this feature in the different commits. I recommend checking them one by one.
implementation details
message-id
which helps with creating ids from messages. I need it in few places that's why it's extracted.compile
and runtime was removed. Context property also stripped in production mode.msgid
field since it against the gettext ideology. But we also don't want to break compatibility with custom ids provided by users. For that purpose during serialization we're storing a flaggenerated-id
in po file, which then used when deserializing to regenerate hash id.input:
#, generated-id msgctxt "my context" msgid "Ola!" msgstr ""
Types of changes
Fixes # (issue)
Checklist