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

Add Chinese Translation #2

Merged
merged 24 commits into from
Jun 19, 2022
Merged

Add Chinese Translation #2

merged 24 commits into from
Jun 19, 2022

Conversation

Bill-Haku
Copy link
Contributor

translation includes:

  • artifacts.json5
  • enemies.json5
  • some of characters.json5

@CLAassistant
Copy link

CLAassistant commented Jun 18, 2022

CLA assistant check
All committers have signed the CLA.

@xicri xicri changed the title Zh translate Add Chinese Translation Jun 18, 2022
dataset/dictionary/enemies.json5 Outdated Show resolved Hide resolved
dataset/dictionary/characters.json5 Outdated Show resolved Hide resolved
dataset/dictionary/characters.json5 Outdated Show resolved Hide resolved
@xicri xicri mentioned this pull request Jun 19, 2022
@Bill-Haku Bill-Haku requested a review from xicri June 19, 2022 02:24
Copy link
Owner

@xicri xicri left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!
When I have fixed tests and CI, I will notify you.

Copy link
Owner

@xicri xicri left a comment

Choose a reason for hiding this comment

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

@Bill-Haku Oh, GitHub Actions pointed out that you use and instead of ". Can you fix it?

@Bill-Haku
Copy link
Contributor Author

@Bill-Haku Oh, GitHub Actions pointed out that you use and instead of ". Can you fix it?

I'm sorry for that, I will check it soon.

@xicri
Copy link
Owner

xicri commented Jun 19, 2022

@Bill-Haku No problem, since I need time for fixing tests & CI, you don't have to hurry.

@Bill-Haku Bill-Haku requested a review from xicri June 19, 2022 03:02
@Bill-Haku
Copy link
Contributor Author

It is fixed and LICENSE has been translated.

dataset/dictionary/enemies.json5 Outdated Show resolved Hide resolved
@Bill-Haku Bill-Haku requested a review from xicri June 19, 2022 03:19
dataset/dictionary/enemies.json5 Outdated Show resolved Hide resolved
@Bill-Haku Bill-Haku requested a review from xicri June 19, 2022 03:52
dataset/dictionary/enemies.json5 Outdated Show resolved Hide resolved
dataset/dictionary/enemies.json5 Outdated Show resolved Hide resolved
dataset/dictionary/enemies.json5 Outdated Show resolved Hide resolved
LICENSE.zh-cn.md Outdated Show resolved Hide resolved
@Bill-Haku
Copy link
Contributor Author

% npm run build

> build
> node scripts/build.js

file:///Users/hakubill/genshin-langdata/node_modules/node-fetch/src/index.js:108
                        reject(new FetchError(`request to ${request.url} failed, reason: ${error.message}`, 'system', error));
                               ^

FetchError: request to https://dataset.genshin-dictionary.com/words.json failed, reason: getaddrinfo ENOTFOUND dataset.genshin-dictionary.com
    at ClientRequest.<anonymous> (file:///Users/hakubill/genshin-langdata/node_modules/node-fetch/src/index.js:108:11)
    at ClientRequest.emit (node:events:520:28)
    at TLSSocket.socketErrorListener (node:_http_client:442:9)
    at TLSSocket.emit (node:events:520:28)
    at emitErrorNT (node:internal/streams/destroy:164:8)
    at emitErrorCloseNT (node:internal/streams/destroy:129:3)
    at processTicksAndRejections (node:internal/process/task_queues:83:21) {
  type: 'system',
  errno: 'ENOTFOUND',
  code: 'ENOTFOUND',
  erroredSysCall: 'getaddrinfo'
}

Node.js v17.4.0

I got this and I think there's no typo in my translation right?.

@xicri
Copy link
Owner

xicri commented Jun 19, 2022

@Bill-Haku Hmm, I modified the DNS record for dataset.genshin-dictionary.com 2 hours ago, so perhaps the change is not applied to DNS servers in China yet. Or GFW blocks it...?

But in the meantime, you can ignore this error because JSON5 is already parsed before accessing https://dataset.genshin-dictionary.com/words.json. This means your JSON5 is probably valid.

@Bill-Haku Bill-Haku requested a review from xicri June 19, 2022 04:56
Copy link
Owner

@xicri xicri left a comment

Choose a reason for hiding this comment

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

All the problems are resolved except for the test!
Thank you, I will work on fixing the error on the test.

@Bill-Haku
Copy link
Contributor Author

@Bill-Haku Hmm, I modified the DNS record for dataset.genshin-dictionary.com 2 hours ago, so perhaps the change is not applied to DNS servers in China yet. Or GFW blocks it...?

But in the meantime, you can ignore this error because JSON5 is already parsed before accessing https://dataset.genshin-dictionary.com/words.json. This means your JSON5 is probably valid.

@xicri Maybe, it was caused by DNS. Anyway, I tried again on my local and nothing unexpected happened. Also, GFW will not block the site as there's nothing illegal or sensitive.

@xicri xicri assigned xicri and Bill-Haku and unassigned xicri Jun 19, 2022
Copy link
Owner

@xicri xicri left a comment

Choose a reason for hiding this comment

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

Sorry for the change request after the approval.
I found some additional wrong translations.

dataset/dictionary/enemies.json5 Outdated Show resolved Hide resolved
dataset/dictionary/enemies.json5 Outdated Show resolved Hide resolved
dataset/dictionary/enemies.json5 Outdated Show resolved Hide resolved
dataset/dictionary/enemies.json5 Outdated Show resolved Hide resolved
dataset/dictionary/enemies.json5 Outdated Show resolved Hide resolved
@Bill-Haku Bill-Haku requested a review from xicri June 19, 2022 11:24
xicri
xicri previously approved these changes Jun 19, 2022
Copy link
Owner

@xicri xicri left a comment

Choose a reason for hiding this comment

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

Thanks, everything is fine 👍

@Bill-Haku
Copy link
Contributor Author

I've followed your instructions but it seems that there's still some problem...

@xicri
Copy link
Owner

xicri commented Jun 19, 2022

89e629e and 3d7e2ea seem unnecessary and I want to remove them. Other commits look good.
How about the following steps to remove the most recent 3 commits and add e525353 again?:

  1. Run git reset HEAD^ three times. (git reset HEAD^ removes the latest commit)
  2. Modify dataset/dictionary/artifacts.json5 to manually add trailing commas like e525353, then commit it.
  3. git push --force origin zh-translate

@xicri
Copy link
Owner

xicri commented Jun 19, 2022

Oops. sorry, I looked carefully and found some other duplicated commits.

@Bill-Haku
Copy link
Contributor Author

It seems that the Github Action Check has passed

@xicri
Copy link
Owner

xicri commented Jun 19, 2022

Yes, the test has passed 🎉
Maybe the code itself has no problem, but too dirty commit history may cause a problem if I have to investigate the history in the future.
I'm not sure why the problem still persists. Can I clean up the commit history on my end?

@Bill-Haku
Copy link
Contributor Author

No problem. Thanks for helping me send this PR. お疲れさまでした!

@xicri
Copy link
Owner

xicri commented Jun 19, 2022

Thanks, I've fixed the commits.

@xicri xicri merged commit fc02da0 into xicri:main Jun 19, 2022
@xicri
Copy link
Owner

xicri commented Jun 19, 2022

Merged. Thanks for your tough work!

I will modify genshin-dictionary repository to optimize the UI to show your Chinese translations next weekend.

It was fun to collaborate with you today. 謝謝!

@Bill-Haku Bill-Haku deleted the zh-translate branch June 19, 2022 14:05
xicri added a commit that referenced this pull request Nov 19, 2022
I found 玻瑞亚斯 appears in-game.
See #2 for the context when we delete this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants