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

Extract text from CLI to enable internationalisation #182

Merged
merged 62 commits into from
May 5, 2021
Merged

Conversation

CarlBeek
Copy link
Collaborator

In order to support internationalisation, the text needs to be removed from the CLI and broken out into separate content files (.json).

This PR implements this via a function that determines the context in which it was called (from the call stack) and uses this information to determine the correct files to load in.

@CarlBeek CarlBeek added enhancement New feature or request do not merge labels Feb 12, 2021
@samajammin
Copy link
Member

@CarlBeek nice!

Just to confirm the file structure, to translate eth2deposit/intl/en/cli/existing_mnemonic.json, we'd upload the translated file e.g. for Spanish as eth2deposit/intl/es/cli/existing_mnemonic.json or e.g. for German eth2deposit/intl/de/cli/existing_mnemonic.json.

@samajammin
Copy link
Member

@wackerow I've tested out uploading nested JSON into Crowdin & can confirm that this format is supported 👍

File uploaded (you'll need to accept my manager invite in order to view):
https://crowdin.com/project/eth2-launchpad-test/settings#files

File translation:
https://crowdin.com/translate/eth2-launchpad-test/4/en-es?filter=basic&value=0

@wackerow
Copy link
Member

wackerow commented Feb 17, 2021

@CarlBeek This looks great! Thanks so much for tackling this.

I took a look through the codebase to find what's remaining to refactor, and will list it here to track:

  • cli/new_mnemonic.py
  • X[ ] key_handling/key_derivation/mnemonic.py
  • X[ ] settings.py
  • deposit.py
  • X[ ] credentials.py
  • X[ ] key_handling/key_derivation/path.py?
  • X[ ] key_handling/key_derivation/tree.py?
  • cli/existing_mnemonic.py
  • cli/generate_keys.py
  • utils/validation.py

Happy to give a hand refactoring the remaining pages, just let me know 👍😀

@CarlBeek
Copy link
Collaborator Author

@wackerow, Thanks for the list. I've tackled the remaining files in 5fceda2.
Regarding:

  • key_handling/key_derivation/mnemonic.py
  • credentials.py
  • key_handling/key_derivation/path.py
  • key_handling/key_derivation/tree.py
    The messages and errors should never be shown to the user. They are there for internal testing & sanity checks. If a user ever sees one of those messages, it means something has gone catastrophically wrong.

settings.py only lists the names of the testnets & mainnet, are we planning on translating these?

@wackerow
Copy link
Member

wackerow commented Feb 23, 2021

@CarlBeek Updated the list above, and looks like that testing bug was fixed? I think the only thing left is a language flag, and an initial prompt if this flag isn't provided (before selecting mnemonic language).

Anything else I'm missing before this is ready?

Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

I got errors while executing the macOS binary:

➜ ./deposit --new-mnemonics
Traceback (most recent call last):
  File "eth2deposit/deposit.py", line 4, in <module>
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 677, in _load_unlocked
  File "/usr/local/lib/python3.7/site-packages/PyInstaller/loader/pyimod03_importers.py", line 623, in exec_module
  File "eth2deposit/cli/existing_mnemonic.py", line 28, in <module>
  File "eth2deposit/utils/intl.py", line 40, in load_text
FileNotFoundError: [Errno 2] No such file or directory: 'eth2deposit/intl/en/cli/existing_mnemonic.json'
[51580] Failed to execute script deposit

I guess the build.spec should include the new json files to datas.

https://github.com/ethereum/eth2.0-deposit-cli/blob/b7f4188633ee89013eb789b6468ef2534019ae54/build_configs/macos/build.spec#L8

eth2deposit/intl/en/cli/existing_mnemonic.json Outdated Show resolved Hide resolved
eth2deposit/intl/en/cli/generate_keys.json Outdated Show resolved Hide resolved
eth2deposit/intl/en/cli/new_mnemonic.json Outdated Show resolved Hide resolved
@@ -0,0 +1,35 @@
{
"validate_password": {
"msg_password_prompt": "Type the password that secures your validator keystore(s)",
Copy link
Member

Choose a reason for hiding this comment

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

Forwarding on a comment by @samajammin on initial intl audit (#180) that I agree with... consider making this clearer that the user is about to create a new password, vs typing an existing password, eg:
Create a password that will secure your validator keystore(s)
"Type the password that secures" kinda implies there is already password currently securing it.

@@ -0,0 +1,14 @@
{
"from_mnemonic": {
"msg_key_creation": "Creating your keys:\t\t"
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible for us to avoid including the tabs (\t) in the JSON files?

Copy link
Member

Choose a reason for hiding this comment

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

We might be able to ignore it in Crowdin but it'd be ideal to remove these to not confuse translators.

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 will look into extraction this from the json and having the tabs encoded in separate print statements. 👍

CarlBeek and others added 2 commits March 8, 2021 11:30
Co-authored-by: Hsiao-Wei Wang <hsiaowei.eth@gmail.com>
Arabic, Romanian, Portuguese (Brazilian), Indonesian, Greek, French, Chinese (simplified), Korean
@wackerow
Copy link
Member

@CarlBeek Hey Carl! Wanted to circle back to this and try to organize final steps. I think the only thing left before was:

  • Language flag / initial prompt

The translations from Crowdin are starting to come in, and the Launchpad side of things is tee'd up for at least 8 of these languages, but one final step is to make sure the CLI commands shown on the website correctly match this repo.

Trying to get an estimate on how much is left here. Should we just temporarily default the CLI commands shown on the Launchpad to English while we finalize things here? Added the languages that are translated so far to a branch off this one, and put it in PR #191.

I also see the Prater testnet was just added... Can't think of how this would delay/change anything, just wanted to make sure.

Almost there!

@CarlBeek CarlBeek requested a review from hwwhww April 15, 2021 08:28
Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

Found some issues during QA. It looks good to me once these issues are fixed.
Awesome work! 🎉

eth2deposit/cli/generate_keys.py Outdated Show resolved Hide resolved
eth2deposit/utils/intl.py Show resolved Hide resolved
eth2deposit/deposit.py Show resolved Hide resolved
eth2deposit/cli/generate_keys.py Outdated Show resolved Hide resolved
@CarlBeek
Copy link
Collaborator Author

CarlBeek commented May 4, 2021

@hwwhww, I've changed quite a bit since you last looked. As far as I can see everything is working perfectly from a translation standpoint. Please can you take one, hopefully final, look 🙏

@CarlBeek CarlBeek requested a review from hwwhww May 4, 2021 14:11
Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

The description of the folder argument + the translations have to be updated; otherwise, LGTM! 👍 👍

README.md Outdated Show resolved Hide resolved
eth2deposit/intl/en/cli/generate_keys.json Outdated Show resolved Hide resolved
eth2deposit/utils/click.py Show resolved Hide resolved
Comment on lines +104 to +114
def validate_int_range(num: Any, low: int, high: int) -> int:
'''
Verifies that `num` is an `int` andlow <= num < high
'''
try:
num_int = int(num) # Try cast to int
assert num_int == float(num) # Check num is not float
assert low <= num_int < high # Check num in range
return num_int
except (ValueError, AssertionError):
raise ValidationError(load_text(['err_not_positive_integer']))
Copy link
Contributor

Choose a reason for hiding this comment

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

It's unclear why it includes low but excludes high. What do you think about including the given upper bound and adjusting the argument:

Suggested change
def validate_int_range(num: Any, low: int, high: int) -> int:
'''
Verifies that `num` is an `int` andlow <= num < high
'''
try:
num_int = int(num) # Try cast to int
assert num_int == float(num) # Check num is not float
assert low <= num_int < high # Check num in range
return num_int
except (ValueError, AssertionError):
raise ValidationError(load_text(['err_not_positive_integer']))
def validate_int_range(num: Any, lower_bound: int, upper_bound: int) -> int::
'''
Verifies that `num` is an `int` and `lower_bound <= num <= upper_bound`
'''
try:
num_int = int(num) # Try cast to int
assert num_int == float(num) # Check num is not float
assert lower_bound <= num_int <= upper_bound # Check num in range
return num_int
except (ValueError, AssertionError):
raise ValidationError(load_text(['err_not_positive_integer']))

and call validate_int_range(num, 0, 2**32 - 1).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Two reasons:

  • It follows python's range/indexing convention (eg x in range(0,8), list[0,8])
  • validate_int_range(num, 0, 2**32) is neater than validate_int_range(num, 0, 2**32 - 1)

'--non_interactive',
default=False,
is_flag=True,
help='Disables interactive prompts.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Unimportant, just FYI it didn't show when I execute python eth2deposit/deposit.py --help. I guess it's because deposit CLI mixes click.option and custom jit_option?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is because I purposefully hid this option, I intended only to use it for testing. (See L47 for hidden argument.)

Are you of the opinion that we should be opening this up to users? (It may be useful to those integrating this into other tools)

@CarlBeek
Copy link
Collaborator Author

CarlBeek commented May 5, 2021

Thanks for your feedback, @hwwhww. I'll merge this now and we can open separate PRs to change validate_int_range() and --non_interactive if we decide this is necessary in the future.

@CarlBeek CarlBeek merged commit a678da8 into dev May 5, 2021
@CarlBeek CarlBeek deleted the translation branch March 16, 2022 11:19
everhusk pushed a commit to earthwallet/earth-wallet-cli that referenced this pull request Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants