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

Terminal supporting controller with multiple languages #1730

Merged
merged 53 commits into from
May 24, 2022
Merged

Conversation

DidierRLopes
Copy link
Collaborator

@DidierRLopes DidierRLopes commented Apr 23, 2022

  • Need to add i18n to requirements and poetry
  • Need to make the language to be selected a feature flag
  • Add documentation on this
  • Still missing helpers for commands. T.b.d. in another PR.

@DidierRLopes DidierRLopes added docs Code documentation feat M Medium T-Shirt size feature labels Apr 23, 2022
@Chavithra Chavithra self-requested a review April 25, 2022 00:45
@DidierRLopes DidierRLopes marked this pull request as ready for review April 25, 2022 01:03
@DidierRLopes
Copy link
Collaborator Author

@piiq @jmaslek what was the issue with pygmentize? Do you remember?

@piiq
Copy link
Contributor

piiq commented Apr 26, 2022

@DidierRLopes There was a Pygments update that was not pushed up the chain. Trying to fix it in #1737

@piiq
Copy link
Contributor

piiq commented Apr 28, 2022

I've looked a bit into this, here are some exploration results

We have the following text that can be translated:

  • Controller menus description and help text
  • Everything wrapped into console.print
  • Table headers in view functions (print_rich_table?)
  • Chart and axis labels

A brief search revealed:
80 controllers
410 unique console.prints (excluding empty lines)
309 print_rich_tables
181 charts

The strings can be parsed with regexp

console\.print\((.*?)\)

or xgettext

xgettext openbb_terminal/stocks/stocks_controller.py -o 1test.txt -L Python -j -a

It would be cool if
a) we could have the translatable string as a key in the translation dictionary. I've tested it with current python-i18n implementation and it works
b) add translation on the console.print level. All text that's passed to console.print gets translated

... but ... f-stings spoil all the fun with this scenario. with f-strings the keys to the translation dictionary would be dynamic.
Although there are ideas online here and here to create wrappers that make f-strings translatable. Maybe we can come up with something?

In addition I don't have a clear understanding of how to maintain without significant effort. Any ideas here are welcome.

@DidierRLopes
Copy link
Collaborator Author

I've looked a bit into this, here are some exploration results

We have the following text that can be translated:

  • Controller menus description and help text
  • Everything wrapped into console.print
  • Table headers in view functions (print_rich_table?)
  • Chart and axis labels

A brief search revealed: 80 controllers 410 unique console.prints (excluding empty lines) 309 print_rich_tables 181 charts

The strings can be parsed with regexp

console\.print\((.*?)\)

or xgettext

xgettext openbb_terminal/stocks/stocks_controller.py -o 1test.txt -L Python -j -a

It would be cool if a) we could have the translatable string as a key in the translation dictionary. I've tested it with current python-i18n implementation and it works b) add translation on the console.print level. All text that's passed to console.print gets translated

... but ... f-stings spoil all the fun with this scenario. with f-strings the keys to the translation dictionary would be dynamic. Although there are ideas online here and here to create wrappers that make f-strings translatable. Maybe we can come up with something?

In addition I don't have a clear understanding of how to maintain without significant effort. Any ideas here are welcome.

Thanks for this @piiq. This all makes sense.

My initial idea was to just translate: controller menu, and command helper (and its arguments). That by itself would already be a very good step forward. Not sure about the ROI of modifying the f-string to .format() since that reduces readability of the code imho.

My initial idea was to have keys for these 3 type of cases as I mention in the contributing.md guidelines. This was mostly because of the maintenance would be easy. But after thinking about this further I think having the English string as key may work best for:

  • readability of the code
  • translating from English to other language as key is text to translate

And my biggest concern can be minimised by introducing a test that checks if all keys on en.yml are being used. If they are not it means that there was a refactoring and the keys need to be adapted. Thoughts?

@Chavithra
Copy link
Contributor

@piiq I will try to summarize the idea let me know if this is accurate.

You are suggesting using English translation as the key ?

@piiq
Copy link
Contributor

piiq commented Apr 29, 2022

You are suggesting using English translation as the key ?

@Chavithra , yep

like i18n.t('search a specific stock ticker for analysis')

rationale:

  • translatable strings would make the code self-documented -> readability doesn't degrade
  • we can set a fallback locale in python-i18n (i18n.set('fallback', 'en')) so no problem if a translation for a language is partial

on the other hand we would need to maintain a dictionary for english and I find it hard to come up with an automation for this at the moment

@DidierRLopes
Copy link
Collaborator Author

You are suggesting using English translation as the key ?

@Chavithra , yep

like i18n.t('search a specific stock ticker for analysis')

rationale:

  • translatable strings would make the code self-documented -> readability doesn't degrade
  • we can set a fallback locale in python-i18n (i18n.set('fallback', 'en')) so no problem if a translation for a language is partial

on the other hand we would need to maintain a dictionary for english and I find it hard to come up with an automation for this at the moment

check my comment above regarding maintainability. I think that's a 10min break task for @colin99d

@DidierRLopes DidierRLopes changed the title Initial implementation of terminal supporting multiple languages Terminal supporting controller with multiple languages May 22, 2022
@DidierRLopes DidierRLopes requested a review from jmaslek May 24, 2022 16:34
Copy link
Collaborator

@jmaslek jmaslek left a comment

Choose a reason for hiding this comment

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

This needs some serious documentation on the contributing guide as to how you add a new feature to the help menu text.

Comment on lines 116 to 142

def init_menu(self, path: str = ""):
self.menu_text = ""
self.menu_path = path

def add_raw(self, raw: str):
self.menu_text += raw

def add_raw_translation(self, raw: str):
self.menu_text += f"{i18n.t(self.menu_path + raw)}"

def add_info_translation(self, info: str):
self.menu_text += f"[info]{i18n.t(self.menu_path + info)}:[/info]\n"

def add_param_translation(self, param: str, value: str):
self.menu_text += f"[param]{i18n.t(self.menu_path + param)}:[/param] {value}\n"

def add_cmd_translation(self, key: str, source: str = "", cond: bool = True):
if source:
source = f" [src][{source}][/src]"
spacing = (22 - (len(key) + 4)) * " "
if cond:
self.menu_text += f"[cmds] {key}{spacing}{i18n.t(self.menu_path + key)}{source}[/cmds]\n"
else:
self.menu_text += f"[unvl] {key}{spacing}{i18n.t(self.menu_path + key)}{source}[/unvl]\n"

def add_menu_translation(self, key: str, cond: bool = True):
Copy link
Collaborator

Choose a reason for hiding this comment

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

no docstrings anywhere

Comment on lines 51 to 75
def __init__(self, path: str = "", col_src: int = 100):
self.menu_text = ""
self.menu_path = path
self.col_src = col_src

def add_raw(self, raw: str):
self.menu_text += raw

def add_raw_translation(self, raw: str):
self.menu_text += f"{i18n.t(self.menu_path + raw)}"

def add_info_translation(self, info: str):
self.menu_text += f"[info]{i18n.t(self.menu_path + info)}:[/info]\n"

def add_param_translation(self, param: str, value: str, spacing: int = 0):
par = i18n.t(self.menu_path + param)
if spacing > len(par):
space = (spacing - len(par)) * " "
else:
space = ""
self.menu_text += f"[param]{par}{space}:[/param] {value}\n"

def add_cmd_translation(self, key: str, source: str = "", cond: bool = True):
spacing = (23 - (len(key) + 4)) * " "
if cond:
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

@DidierRLopes
Copy link
Collaborator Author

This needs some serious documentation on the contributing guide as to how you add a new feature to the help menu text.

This needs some serious documentation on the contributing guide as to how you add a new feature to the help menu text.

Yup, was missing it. Now it should be good.

Let me know what you think

@DidierRLopes DidierRLopes merged commit 707fd8f into main May 24, 2022
@piiq piiq deleted the lang branch May 25, 2022 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Code documentation feat XL Extra Large feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants