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

refactor: fix I18n class constructor jsdoc to resolve vscode type errors #3195

Merged
merged 4 commits into from
Sep 14, 2023
Merged

refactor: fix I18n class constructor jsdoc to resolve vscode type errors #3195

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c321e8d
refactor: fix I18n class constructor jsdoc to resolve vscode type errors
qwerty541 Sep 3, 2023
7b3c991
Merge remote-tracking branch 'upstream/master' into refactor_fix_i18n…
qwerty541 Sep 4, 2023
515a183
Merge remote-tracking branch 'upstream/master' into refactor_fix_i18n…
qwerty541 Sep 9, 2023
d10840c
Merge remote-tracking branch 'upstream/master' into refactor_fix_i18n…
qwerty541 Sep 14, 2023
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
2 changes: 1 addition & 1 deletion src/common/I18n.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ class I18n {
* Constructor.
*
* @param {Object} options Options.
* @param {string} options.locale Locale.
* @param {string=} options.locale Locale.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@qwerty541 I checked this change but could not find what rule this is fixing. Isn't the locale property required for the function to work?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@qwerty541 I now see that the default value is hidden inside the constructor. Maybe we can replace it with the following:

constructor({ locale = FALLBACK_LOCALE, translations }) {
    this.locale = locale;
    this.translations = translations;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Additionally, the t method could be improved:

t(str) {
    const { translations, locale } = this;
    const translation = translations[str]?.[locale];
    if (!translation) {
        throw new Error(`'${str}' translation not found for locale '${locale}'`);
    }
    return translation;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rickstaa This pull request resolves the following vscode type errors:

src/cards/repo-card.js on line 94
src/cards/stats-card.js on line 150
src/cards/top-languages-card.js on line 744
src/cards/wakatime-card.js on line 208

This occurs because locale is optional query parameter and can be undefined and I18n class constructor jsdoc requires string.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Additionally, the t method could be improved:

t(str) {
    const { translations, locale } = this;
    const translation = translations[str]?.[locale];
    if (!translation) {
        throw new Error(`'${str}' translation not found for locale '${locale}'`);
    }
    return translation;
}

I like current variant of this methos more because it contains different error messages when locale and translation is missing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Additionally, the t method could be improved:

t(str) {
    const { translations, locale } = this;
    const translation = translations[str]?.[locale];
    if (!translation) {
        throw new Error(`'${str}' translation not found for locale '${locale}'`);
    }
    return translation;
}

I like current variant of this methos more because it contains different error messages when locale and translation is missing.

No problem, both are good! How about moving the fallback into the object deconstruction 🤔?

Moving the fallback into the object deconstruction looks good for me, but i have doubts about if it should be done inside this pull request or inside a new one, since this one about fixing jsdoc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also thank you for following, i appreciate it, followed you back.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Moving the fallback into the object deconstruction looks good for me, but i have doubts about if it should be done inside this pull request or inside a new one, since this one about fixing jsdoc.

Let's do this in a new PR.

Also thank you for following, i appreciate it, followed you back.

Of course. I'm curious about your projects and contributions 🚀!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Recently i tried to make change with moving the fallback into the object deconstruction. Looks like this does not suit us because when locale variable passed into constructor with undefined value it override the default value bacause undefined is valid. Currently it will be handled by the following code this.locale = locale || FALLBACK_LOCALE;.

Moving fallback can de done only the following way:

constructor({ locale = FALLBACK_LOCALE, translations }) {
    // Check if 'locale' is undefined and set it to the default value if true
    if (locale === undefined) {
      locale = FALLBACK_LOCALE;
    }

    this.locale = locale;
    this.translations = translations;
  }

But i do not think that code will be better after this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree. Thanks for testing my improvement. I forgot about the fact that undefined is seen as a valid input! I think we can leave the old code 👍🏻.

* @param {Object} options.translations Translations.
*/
constructor({ locale, translations }) {
Expand Down