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

Allow to translate empty strings #24

Merged
merged 2 commits into from
Jun 26, 2023

Conversation

VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented Jun 8, 2023

Hi @JanEbbing

Currently an exception is thrown when we try to translate an empty string.
This seems to be an unnecessary check because if I remove the check manually in my vendors, the API works fine.

When receiving multiple users inputs and if we want to translate for instance

['foo', 'bar', '', 'baz']

This seems unnecessary to ask us to

  • Filter the '' value
  • Translate [0 => 'foo', 1 => 'bar', 3 => 'baz'] (while the API call lose the initial keys)
  • Receving the translation [0 => 'Tfoo', 1 => 'Tbar', 2 => 'Tbaz']
  • Insert back the empty string in the right position to get [0 => 'Tfoo', 1 => 'Tbar', 2 => '', 3 => 'Tbaz']

While an API call with ['foo', 'bar', '', 'baz'] seems to works fine if the input validation is removed.

@JanEbbing
Copy link
Member

Thanks for this! I agree the behaviour is inconsistent, e.g. in deepl-python translating '' raises an error but translating [''] works fine. I'm checking internally why we don't allow empty string and how we want to handle this, let me get back to you.

@JanEbbing
Copy link
Member

Agreed that we should allow translating empty string. I'll merge this

Copy link
Member

@JanEbbing JanEbbing left a comment

Choose a reason for hiding this comment

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

Nit: For the future, we use conventional commits, so commit messages should start with something like fix:, docs:, feat: etc
Also we keep a CHANGELOG.md file (I'm looking to automate that in the future based on commit messages, as we have the classification already)

@JanEbbing JanEbbing merged commit 949a528 into DeepLcom:main Jun 26, 2023
@VincentLanglet VincentLanglet deleted the allow-empty-text branch June 26, 2023 08:54
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