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

Feature choose language #22

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

alonmar
Copy link

@alonmar alonmar commented Aug 1, 2022

This issue added in doc #21

  • 💡Added command to choose the language of quotes
  • 💡Translate quotes to spanish
  • 💡Moved quotes to new folders according to language
  • 💡Added test language command
  • 💡Default language is english

I tried to copy as much as possible to your coding/commit style, I await your comments 👀

@guedesfelipe
Copy link
Owner

I don't know if this is the best way to have the translations, I was thinking of leaving the translations out of the python package to keep the package smaller and if the user wants a specific translation, the CLI would download the translation and put it inside the folder of configuration.

Today we only have two languages, but in the future we may have several and this can increase the size of the package and not everyone needs to have all the translations by default.

What do you think?

@guedesfelipe
Copy link
Owner

To fix the linting you can run the make format command 😃

@alonmar
Copy link
Author

alonmar commented Aug 2, 2022

Yeah, I think it is better to keep it to a minimum, so can we continue with my PR for the moment which is 2 languages (i am fixed the linting), and start looking for an option to increase the number of languages?

@guedesfelipe
Copy link
Owner

I think it's better to do the final solution to avoid problems with future versions.

I thought of having a folder called translations at the root of the project and inside it containing the folders or just the files with the translated quotes. In the CLI, have a command that lists the possible translations and downloads from this location to the config folder of the PLS-CLI and configures the default language within the configuration file.

@@ -142,16 +142,16 @@ def language(lang: str) -> None:
else:
settings['language'] = lang
Settings().write_settings(settings)
# Here function Dowload quotes

# Here function Dowload quotes
Copy link
Author

Choose a reason for hiding this comment

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

@guedesfelipe i am reformat the code, the download part would go here, but i really don't have much idea how to query the current branch to be able to download the quotes, can you give me a hint?

Copy link
Owner

Choose a reason for hiding this comment

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

Sure! I will take a look at the code and help you with that as soon as possible

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry for the delay, I was busy these days and I still haven't been able to make an example code, but I was thinking of doing the part of downloading the translated files like this:
https://github.com/Textualize/rich/blob/master/examples/downloader.py

Or with Requests library...

@alonmar
Copy link
Author

alonmar commented Aug 4, 2022

-💡Moved quotes location
-💡Remove language selection from setup command
-💡Add the command and function that lists the languages

@@ -131,6 +131,70 @@ def quotes(show: bool = True) -> None:
)


@app.command('language', rich_help_panel='Utils and Configs')
Copy link
Owner

Choose a reason for hiding this comment

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

Here I suggest to using enum, like this:
https://typer.tiangolo.com/tutorial/parameter-types/enum/

Copy link
Author

Choose a reason for hiding this comment

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

That was a better solution, it's already updated 👨‍💻

@@ -131,6 +132,45 @@ def quotes(show: bool = True) -> None:
)


class language(str, Enum):
english = "english"
Copy link
Owner

Choose a reason for hiding this comment

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

I think it's better to use the short form, like en for english and es for spanish, what do you think?

) -> None:
"""Choose language 🛩️"""
settings = Settings().get_settings()
if settings["language"] == lang:
Copy link
Owner

Choose a reason for hiding this comment

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

here it would be better to have just an if with the comparison of different. And in the end have only one center_print to avoid code duplication, like this:

if settings["language"] != lang:
    settings["language"] = lang
    Settings().write_settings(settings)
    # Here function Dowload quotes
    center_print(
        Rule(
            "Thanks for letting me know that!",
            style=insert_or_delete_line_style,
        ),
        style=insert_or_delete_text_style,
    )

cener_print(
    Rule(
        f"Current language is: {lang}",
        style=insert_or_delete_line_style,
    ),
    style=insert_or_delete_text_style,
)

@@ -481,6 +521,18 @@ def setup() -> None:
)
console.print(code_markdown)

code_markdown = Markdown(
"""
pls language --lang <language>
Copy link
Owner

Choose a reason for hiding this comment

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

Here is correct? pls language --lang <language>

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.

2 participants