Add translate_from feature to translator_cog and its tests#164
Add translate_from feature to translator_cog and its tests#164Eyad-Jawad wants to merge 8 commits into
Conversation
Eyad-Jawad
commented
Jul 6, 2026
- 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
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Can you follow our testing naming convention:
test_<action>__expect_<result> and test_<action>__with_<modifier>__expect_<result>
There was a problem hiding this comment.
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_translatorbecomestest_translator__expect_embed_with_translationtest_language_autocomplete_ara_inputbecomestest_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.
| ctx: Context, | ||
| *, | ||
| sentence: str, | ||
| translate_from: str | None = "auto", |
There was a problem hiding this comment.
If we default to "auto", I don't think it's relevant to allow None:
translate_from: str = "auto"| *, | ||
| sentence: str, | ||
| translate_from: str | None = "auto", | ||
| translate_into: str, |
There was a problem hiding this comment.
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"