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(stdlib,exts): implement Tr #578

Merged
merged 6 commits into from
Nov 17, 2020
Merged

feat(stdlib,exts): implement Tr #578

merged 6 commits into from
Nov 17, 2020

Conversation

lkipke
Copy link
Collaborator

@lkipke lkipke commented Nov 11, 2020 •
edited
Loading

Summary

This implements the stdlib Tr function (link to docs). Closes #496

@lkipke lkipke added enhancement Any addition to this project's existing capabilities e2e Affects this project's end-to-end test cases (the BrightScript sample files executed during testing) stdlib Affects the standard library included with this BrightScript implementation documentation Affects how this project is documented; includes clarifications, corrections, and additions labels Nov 11, 2020
@lkipke lkipke self-assigned this Nov 11, 2020
lkipke
lkipke commented Nov 11, 2020
View reviewed changes
src/stdlib/Localization.ts
import { XmlDocument } from "xmldoc";

/**
* Supported locales in RBI.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know that we need to enforce this, we could just grab whatever subdirectories exist in the locale/ directory. But it made traversal a little easier 🤷

alimnios72
alimnios72 reviewed Nov 13, 2020
View reviewed changes
Copy link
Collaborator

@alimnios72 alimnios72 left a comment

Choose a reason for hiding this comment

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

I have a few questions but this is looking great

src/brsTypes/components/RoDeviceInfo.ts
* locale has been specified, use that. Otherwise, default to the Node process.
*/
public static get locale(): string {
return RoDeviceInfo._locale || process.env.LOCALE || "";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

src/brsTypes/components/RoDeviceInfo.ts Show resolved Hide resolved
test/e2e/resources/components/localization/source/main.brs Show resolved Hide resolved
src/stdlib/Localization.ts Show resolved Hide resolved
src/stdlib/Localization.ts Show resolved Hide resolved
sjbarag
sjbarag approved these changes Nov 13, 2020
View reviewed changes
Copy link
Owner

@sjbarag sjbarag left a comment

Choose a reason for hiding this comment

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

Looks great! We may be missing a few bits of the TS file spec (and we don't seem to support XLIFF files yet) but we can handle those in a separate PR as needed.

Nice work @lkipke !

test/e2e/resources/components/localization/source/main.brs Show resolved Hide resolved
@lkipke
Copy link
Collaborator Author

lkipke commented Nov 17, 2020

Looks great! We may be missing a few bits of the TS file spec (and we don't seem to support XLIFF files yet) but we can handle those in a separate PR as needed.

Yep, that was my thought as well 👍

alimnios72
alimnios72 approved these changes Nov 17, 2020
View reviewed changes
Copy link
Collaborator

@alimnios72 alimnios72 left a comment

Choose a reason for hiding this comment

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

I didn't have enough coffee when checking this PR yesterday, things look great!

@lkipke lkipke merged commit 1abe700 into sjbarag:main Nov 17, 2020
@lkipke lkipke deleted the features/implement-translation branch November 17, 2020 18:32
@nadiapadalka
Copy link

nadiapadalka commented Nov 25, 2020 •
edited by sjbarag
Loading

Hi @lkipke , I've tried to call Tr without mocking but I'm receiving the translations source like: "HELLO_WORLD" but not the translated text "Hello, world!", error is not present anymore, but looks like Tr not working properly(

@sjbarag
Copy link
Owner

sjbarag commented Nov 25, 2020

Hey @nadiapadalka — please don't post references to confidential materials here, including translations. I'm redacting parts of your comment.

@sjbarag
Copy link
Owner

sjbarag commented Nov 25, 2020

Hi @lkipke , I've tried to call Tr without mocking but I'm receiving the translations source like: "HELLO_WORLD" but not the translated text "Hello, world!", error is not present anymore, but looks like Tr not working properly(

@nadiapadalka That may be solved by #587 , which was released in v0.39.0. Please try again with that version of the interpreter. If you still see that error, please file a new issue with the simplest file(s) you can find that reproduce the issue 😃

@nadiapadalka
Copy link

Hey @nadiapadalka — please don't post references to confidential materials here, including translations. I'm redacting parts of your comment.

@sjbarag , sure, sorry

@nadiapadalka
Copy link

nadiapadalka commented Nov 25, 2020 •
edited
Loading

Hi @lkipke , I've tried to call Tr without mocking but I'm receiving the translations source like: "HELLO_WORLD" but not the translated text "Hello, world!", error is not present anymore, but looks like Tr not working properly(

@nadiapadalka That may be solved by #587 , which was released in v0.39.0. Please try again with that version of the interpreter. If you still see that error, please file a new issue with the simplest file(s) you can find that reproduce the issue 😃

thanks @sjbarag ok, I will try

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

@alimnios72 alimnios72 alimnios72 approved these changes

@sjbarag sjbarag sjbarag approved these changes

Assignees

@lkipke lkipke

Labels
documentation Affects how this project is documented; includes clarifications, corrections, and additions e2e Affects this project's end-to-end test cases (the BrightScript sample files executed during testing) enhancement Any addition to this project's existing capabilities stdlib Affects the standard library included with this BrightScript implementation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Brs doesn't support the native tr() function
4 participants
@lkipke @nadiapadalka @sjbarag @alimnios72