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

chore: add locale sort/check script #5339

Merged
merged 5 commits into from
Mar 24, 2023
Merged

Conversation

floyd-li
Copy link
Member

Thank you for your contribution to the KodaDot NFT gallery.

anyone has idea on how to integrate to CI? 🤔️

what this script do:

  • sort the locale json keys by alphabet
  • check other locales missing keys compared with base locale (I set en as default)

👇 _ Let's make a quick check before the contribution.

PR Type

  • Bugfix
  • Feature
  • Refactoring

Context

Before submitting pull request, please make sure:

  • My contribution builds clean without any errors or warnings
  • I've merged recent default branch -- main and I've no conflicts
  • I've tried to respect high code quality standards
  • I've didn't break any original functionality
  • I've posted a screenshot of demonstrated change in this PR

Optional

  • I've tested it at </bsx/collection>
  • I've tested PR on mobile
  • I've written unit tests 🧪
  • I've found edge cases

Had issue bounty label?

  • Fill up your KSM address: Payout

Community participation

Screenshot 📸

  • My fix has changed something on UI; a screenshot is best to understand changes for others.

@floyd-li floyd-li requested a review from a team as a code owner March 23, 2023 12:50
@floyd-li floyd-li requested review from roiLeo and Jarsen136 and removed request for a team March 23, 2023 12:50
@kodabot
Copy link
Collaborator

kodabot commented Mar 23, 2023

WARNING @floyd-li PR for issue #4868 which isn't assigned to you. Please be warned that this PR may get rejected if there's another assignee for issue #4868

@netlify
Copy link

netlify bot commented Mar 23, 2023

Deploy Preview for koda-nuxt ready!

Name Link
🔨 Latest commit 7e6d289
🔍 Latest deploy log https://app.netlify.com/sites/koda-nuxt/deploys/641d49f9a298080008d13c5d
😎 Deploy Preview https://deploy-preview-5339--koda-nuxt.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Contributor

@Jarsen136 Jarsen136 left a comment

Choose a reason for hiding this comment

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

Pls also commit the changes on locales/** after execute the script.

table.push([key, checkResult[key]])
})
console.table(table)
throw new Error('Missing Translations')
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we need to throw an error when executing this script. We do know that some translation keys are missing in some language files.

Copy link
Contributor

Choose a reason for hiding this comment

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

throw which key in which file by local, please :)

Copy link
Member Author

Choose a reason for hiding this comment

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

hmmm, I plan to add this script to the commit hook, but seems now we are missing plenty of keys with other locales

Copy link
Member Author

Choose a reason for hiding this comment

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

@roiLeo actually it will display the message in console like this
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Compared to eslint command we can't see the the full list here, so I'll have to run it again and again to check & fix missing keys

Copy link
Member Author

Choose a reason for hiding this comment

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

hmmm yes cuz we are missing a lot of translations now...
maybe we can generate a log.json file to log all the missing locales and keys?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes! or add a parameter to check specific file

Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

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

Wow!
This look pretty cool

maybe we can improve table columns with more details

Integrate to CI can come later, nice work ;)

script/checkLocale.mjs Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@floyd-li
Copy link
Member Author

just update, and it now supported parameter
usage: pnpm locales:check --locales=en,fr,de,es --baseLocale=en, while the default locale=all and baseLocale=en
and when i tried to check all locales, it get an error while parse JSON of sk.json. seems this locale got specific character? anyone got idea?
image

@roiLeo
Copy link
Contributor

roiLeo commented Mar 23, 2023

just update, and it now supported parameter usage: pnpm locales:check --locales=en,fr,de,es --baseLocale=en, while the default locale=all and baseLocale=en and when i tried to check all locales, it get an error while parse JSON of sk.json. seems this locale got specific character? anyone got idea?

Maybe bad file format (UTF-8 BOM), should be utf-8

@codeclimate
Copy link

codeclimate bot commented Mar 24, 2023

Code Climate has analyzed commit 7e6d289 and detected 0 issues on this pull request.

View more on Code Climate.

@floyd-li floyd-li requested review from roiLeo and Jarsen136 and removed request for roiLeo and Jarsen136 March 24, 2023 07:00
Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

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

I don't know why but when I run the command it changes all locales files could you take a look?

@floyd-li
Copy link
Member Author

I don't know why but when I run the command it changes all locales files could you take a look?

@roiLeo it's expected. this script will check and sort the translation keys

await resortJson()

const checkResult = await checkLocale()

Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

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

❤️ top! nice work

✅ wfm

@roiLeo roiLeo added the S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked label Mar 24, 2023
@floyd-li floyd-li requested a review from Jarsen136 March 24, 2023 09:12
@vikiival
Copy link
Member

woah

@vikiival
Copy link
Member

pay 30 usd

@vikiival vikiival merged commit 16a3775 into kodadot:main Mar 24, 2023
@yangwao
Copy link
Member

yangwao commented Mar 24, 2023

😍 Perfect, I’ve sent the payout
💵 $30 @ 33.85 USD/KSM ~ 0.886 $KSM
🧗 EZiu1PjV2j2JHKxY6mHnFwwCRCoV27uHKQSkKXATSh1srJT
🔗 0xd875a0fb92796ce7e196a884ce43c13411d514cd3069a691c092a64a2767262b

🪅 Let’s grab another issue and get rewarded!
🪄 github.com/kodadot/nft-gallery/issues

@yangwao yangwao added the paid pull-request has been paid label Mar 24, 2023
@floyd-li floyd-li deleted the local-check-script branch October 8, 2023 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
paid pull-request has been paid S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node script to organize locales files
6 participants