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

Allow loading language files with two part language code #339

Merged
merged 3 commits into from
Apr 22, 2022
Merged

Allow loading language files with two part language code #339

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cf81cbe
Change the imported filenames to inlcude underscore in two part langu…
TPiUnikie Apr 19, 2022
37aaa40
Refactoring the code
TPiUnikie Apr 20, 2022
d682b9d
Refactored code and added comment
TPiUnikie Apr 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion src/language-helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,26 @@ export class AppLocalization {
this.resetLocalizedUI();
}

private denormalize(locale: string): string {
if (locale === "en") {
locale = "en-EN";
}
if (locale.indexOf("-") >= 0) {
const langDesc = 1;
const partsReq = 2;
const part = locale.split("-");
if (part.length >= partsReq) {
part[langDesc] = part[langDesc].toUpperCase();
}
return part.join("_");
}
return locale;
Copy link
Member

Choose a reason for hiding this comment

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

A few things about this function:

  • The if branch is redundant? The code only does the split if the string contains a hyphen, so we know we'll get at least two parts.
  • Having langDesc & partsReq as defined constants I think might be adding more confusion. I'd probably just use the numbers straight in the code (if partsReq is even necessary).
  • Maybe parts instead of part for the variable - it's all the parts of the split string
  • Could it get a comment saying what it does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • The first if was like a guard to avoid variables to be reserved if there is no hyphen, maybe those will be optimized out?
  • maybe langDesc rename to localeIndex. Currently the second part is for uppercase.
    If there are cases like aa_bb_cc, then it would output aa-BB-cc, using last index: aa-bb-CC.
  • Parts would be a better name
  • partsReq can be removed, if change the > langDesc is changed to comparison

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, although I'm not sure the if is really optimising anything: variables are very cheap, so this will probably spend more time searching through the string twice. In any case, it would be a tiny optimisation: code legibility is much more important. I don't really mind though, although if you do want to do it then we should do it with the more modern includes() rather than indexOf().

Also I'm afraid I still don't understand the need for the localeSubIndex const: any actual constants should be near the top of the file above the class, but for the second thing in an array you can just say parts[1] which is clearer and more concise.

A comment on the function saying what it does would still be very useful ('denormalize' doesn't really tell me what the function is supposed to do).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comment
Removed the 'if' check
Used fixed index numbers instead of defined constant

}

public fetchTranslationJson(locale: string): Record<string, string> {
try {
console.log("Fetching translation json for locale: " + locale);
return require(`./i18n/strings/${locale}.json`);
return require(`./i18n/strings/${this.denormalize(locale)}.json`);
} catch (e) {
console.log(`Could not fetch translation json for locale: '${locale}'`, e);
return null;
Expand Down