Skip to content

Add translate_from feature to translator_cog and its tests#164

Open
Eyad-Jawad wants to merge 8 commits into
Code-Society-Lab:mainfrom
Eyad-Jawad:addTranslateFromToTranslationCog
Open

Add translate_from feature to translator_cog and its tests#164
Eyad-Jawad wants to merge 8 commits into
Code-Society-Lab:mainfrom
Eyad-Jawad:addTranslateFromToTranslationCog

Conversation

@Eyad-Jawad

Copy link
Copy Markdown
  • Add the feature on issue #127, which is to add translate from to translator_cog
  • Add tests for translator_cog and the helper function language_autocomplete
  • Add conftest.py to be used for all test files

@chrisdedman chrisdedman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM. Other than the test method that does not follow our convention (please, make changes), I am good with it. Please, @PenguinBoi12 have a second review.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can you follow our testing naming convention:
test_<action>__expect_<result> and test_<action>__with_<modifier>__expect_<result>

@PenguinBoi12 PenguinBoi12 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In general it looks good to me but has @chrisdedman pointed out, we prefer when test follow this convention: test_<function/method/action>__expect_<result> and test_<function/method/action>__<modifier>__expect_<result>

For example:

  • test_translator becomes test_translator__expect_embed_with_translation
  • test_language_autocomplete_ara_input becomes test_language_autocomplete__with_partial_input__expect_matching_languages

You also need to test both the happy paths (test works as intended) and unhappy paths. For example, you should test what happens when the translators receive a bad input (like a language that doesn't exists). Many of your test do not test those correctly.

Otherwise, good job! It's a good first open-source contribution.

Comment thread bot/extensions/translator_cog.py Outdated
ctx: Context,
*,
sentence: str,
translate_from: str | None = "auto",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we default to "auto", I don't think it's relevant to allow None:

translate_from: str = "auto"

Comment thread bot/extensions/translator_cog.py Outdated
*,
sentence: str,
translate_from: str | None = "auto",
translate_into: str,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think, since we can now add translate_from , it could be cool to add english as a default to translate_into:

translate_from: str = "english"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Translation Cog - Add the ability to choose the translation from

3 participants