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

Allow splitting translations #52

Merged
merged 14 commits into from
Sep 24, 2024
Merged

Allow splitting translations #52

merged 14 commits into from
Sep 24, 2024

Conversation

strbit
Copy link
Contributor

@strbit strbit commented Aug 29, 2024

Closes #51

As explained in the issue referenced above, sometimes it becomes too difficult to maintain translation files for a project and currently the library does not allow files to be split into different directories or files.

This pull-request implements this feature by walking every .ftl file in the locales directory, reading it, and merging it with any other translation it finds for that locale. This might not be the best way of implementing this feature... but it works. It also works with the standard translation layout currently implemented, which means users can use both types of layouts at once.

As of writing this, I haven't discovered anything that would be broken by this pull-request, please do let me know if it does in fact cause breaking changes or if any issues were discovered.

@KnorpelSenf
Copy link
Member

I'll try to review this tonight

Copy link
Member

@EdJoPaTo EdJoPaTo left a comment

Choose a reason for hiding this comment

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

Haven't read everything as I had limited time, just some comments so far from glancing over it.

examples/locales/de.ftl Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/deps.ts Outdated Show resolved Hide resolved
src/i18n.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
Copy link
Member

@KnorpelSenf KnorpelSenf left a comment

Choose a reason for hiding this comment

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

This looks really good to me, thanks for the contribution! Could you add some tests? I think we can merge right away once this is done.

Changed `NestedTranslation` from `type` to `interface`.
README.md Outdated Show resolved Hide resolved
@strbit
Copy link
Contributor Author

strbit commented Sep 12, 2024

@KnorpelSenf I've changed the makeTempLocalesDir utility function so that it creates a couple of split translations for other tests to use, if you had a different idea in mind for what tests to add, please let me know and I'll try to implement them.

Copy link
Member

@EdJoPaTo EdJoPaTo left a comment

Choose a reason for hiding this comment

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

The docs of it are in a good shape already!

I quickly glanced over the code but @KnorpelSenf looked into that far more deeply than I did. So I cant say much about it.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@KnorpelSenf KnorpelSenf left a comment

Choose a reason for hiding this comment

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

Thank you!

@KnorpelSenf KnorpelSenf merged commit dfc211a into grammyjs:main Sep 24, 2024
1 check passed
@strbit
Copy link
Contributor Author

strbit commented Sep 25, 2024

Thanks for merging the pull-request, are there any details on when the new version will be released?

@KnorpelSenf
Copy link
Member

@rojvv said he wanted to do it “later” … I believe that means it'll happen today at some point. If that doesn't happen, feel free to ping me again

@strbit
Copy link
Contributor Author

strbit commented Sep 25, 2024

Alright, no worries if it takes a couple of days.

@rojvv
Copy link
Member

rojvv commented Sep 25, 2024

Available in 1.1.0.

@strbit
Copy link
Contributor Author

strbit commented Sep 25, 2024

Apologies if I'm misunderstanding something, but was this version released correctly? I just tried to upgrade and I got a lot of errors. I looked at the source on NPM and saw that there is no src folder.

folder

@rojvv
Copy link
Member

rojvv commented Sep 25, 2024

Apologies for the inconvenience. 1.1.2 should work.

@strbit
Copy link
Contributor Author

strbit commented Sep 25, 2024

Works as expected, thanks!

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.

Allow "splitting" translations?
4 participants