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.

Sign up for GitHub

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

Jump to bottom

feat: support pot format for the translation files #124

Merged
merged 6 commits into from
Apr 7, 2022
Merged

feat: support pot format for the translation files #124

merged 6 commits into from
Apr 7, 2022

Conversation

orestisioakeimidis
Copy link
Contributor

Add a new configuration option to support multiple formats (json, pot) for the translation files. The new option defaults to json and it shouldn't introduce any breaking changes.

Closes #45

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@orestisioakeimidis orestisioakeimidis mentioned this pull request Feb 14, 2022
Closed
shaharkazaz
shaharkazaz reviewed Feb 14, 2022
View reviewed changes
Copy link
Collaborator

@shaharkazaz shaharkazaz left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!
I made some first initial comments, I'll review when I have more time

README.md Outdated Show resolved Hide resolved
__tests__/buildTranslationFiles.spec.ts Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
src/keys-builder/utils/create-translation.ts Outdated Show resolved Hide resolved
src/keys-builder/utils/create-translation.ts Outdated Show resolved Hide resolved
@shaharkazaz shaharkazaz added the good first issue Good for newcomers label Feb 14, 2022
@orestisioakeimidis
Copy link
Contributor Author

@shaharkazaz PR has been updated. Looking forward to your response!

@orestisioakeimidis
Copy link
Contributor Author

@shaharkazaz Do we have any updates on this one? Thanks in advance!

@shaharkazaz
Copy link
Collaborator

@orestisioakeimidis hi! I'm swamped, I didn't have the time to re-review.
I'll try to find some time this weekend 🙏

shaharkazaz
shaharkazaz reviewed Mar 11, 2022
View reviewed changes
Copy link
Collaborator

@shaharkazaz shaharkazaz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great work! 🔥

__tests__/buildTranslationFiles.spec.ts Outdated Show resolved Hide resolved
__tests__/buildTranslationFiles.spec.ts Show resolved Hide resolved
__tests__/buildTranslationFiles.spec.ts Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
src/keys-builder/build-translation-file.ts Outdated Show resolved Hide resolved
src/keys-builder/utils/get-current-translation.ts Outdated Show resolved Hide resolved
src/keys-builder/utils/get-current-translation.ts Outdated Show resolved Hide resolved
src/keys-builder/utils/get-current-translation.ts Outdated Show resolved Hide resolved
src/keys-builder/utils/scope.utils.ts Show resolved Hide resolved
src/keys-detective/compare-keys-to-files.ts Outdated Show resolved Hide resolved
@shaharkazaz
Copy link
Collaborator

@orestisioakeimidis Just making sure you saw the comments 😄

@orestisioakeimidis
Copy link
Contributor Author

Hey @shaharkazaz, yes I saw them. Thanks for picking it up! I'll try to have a look this week.

@orestisioakeimidis
Copy link
Contributor Author

Pushed new changes and responded to a few comments @shaharkazaz.

@shaharkazaz
Copy link
Collaborator

@orestisioakeimidis not sure if you noticed, I left new comments 🙂

@orestisioakeimidis
Copy link
Contributor Author

@shaharkazaz I've seen them, but didn't have the time to work on it yet. Will try to do it asap. Thanks for checking!

@shaharkazaz
Copy link
Collaborator

@orestisioakeimidis Any chance you can pick this up? it's really almost done! I want to merge this 🔥

@orestisioakeimidis
Copy link
Contributor Author

I'll do it tomorrow @shaharkazaz. I've been feeling sick last week and didn't have the chance.

@shaharkazaz
Copy link
Collaborator

@orestisioakeimidis sure thing man! Hope everything is well 🙏

@orestisioakeimidis
Copy link
Contributor Author

@shaharkazaz pushed a change and responded in one of the comments.

shaharkazaz
shaharkazaz reviewed Apr 7, 2022
View reviewed changes
src/keys-builder/utils/create-translation.ts Outdated Show resolved Hide resolved
src/keys-builder/utils/create-translation.ts Outdated Show resolved Hide resolved
src/keys-builder/utils/get-current-translation.ts Outdated Show resolved Hide resolved
src/keys-builder/utils/get-current-translation.ts Outdated Show resolved Hide resolved
src/keys-builder/utils/scope.utils.ts Show resolved Hide resolved
@shaharkazaz
Copy link
Collaborator

@orestisioakeimidis Left a few last comments, note that you need to update from master 🙂

@orestisioakeimidis
feat: support pot format for the translation files
c7b6c9f 
Add a new configuration option to support multiple formats (json, pot) for the translation files.
The new option defaults to json and it shouldn't introduce any breaking changes.

Closes jsverse#45
@orestisioakeimidis
change name to output-format and improve tests
cda8391
@orestisioakeimidis
update gettext-parser and commit lock change
33d3a42
@orestisioakeimidis
use object for params and improve tests
205d29a
@orestisioakeimidis
add warning for the provided file in the po parse
36af056
@orestisioakeimidis
Copy link
Contributor Author

@shaharkazaz pushed changes and updated from master!

@shaharkazaz
Copy link
Collaborator

@orestisioakeimidis Great work 👍

@shaharkazaz shaharkazaz merged commit 658c4b0 into jsverse:master Apr 7, 2022
@shaharkazaz
Copy link
Collaborator

@orestisioakeimidis Note that I made some refactors in d2c84d9 and renamed the input to file format.
I also made several type changes to make the code stricter. You are welcome to take a look 🙂

@orestisioakeimidis
Copy link
Contributor Author

It looks great @shaharkazaz! Only one thing you missed is the outputFormat name in the README file. Make sure to fix it! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shaharkazaz shaharkazaz shaharkazaz approved these changes

Assignees
No one assigned
Labels
good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

po file support
2 participants
@orestisioakeimidis @shaharkazaz