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.

Sign up for GitHub

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

Jump to bottom

Add filename param to Translator#translate_document() method #30

Merged
merged 2 commits into from
Jun 10, 2022
Merged

Add filename param to Translator#translate_document() method #30

merged 2 commits into from
Jun 10, 2022

Conversation

seratch
Copy link
Contributor

@seratch seratch commented Apr 3, 2022

Thanks for making this great SDK!

When using Translator#translate_document() method with BinaryIO input_document argument, the internal translate_document_upload call fails due to the following exception:

  File "/path-to-ptyhon/lib/python3.9/site-packages/deepl/translator.py", line 827, in translate_document
    handle = self.translate_document_upload(
  File "/path-to-ptyhon/lib/python3.9/site-packages/deepl/translator.py", line 892, in translate_document_upload
    raise ValueError(
ValueError: filename is required if uploading file content as string or bytes

This pull request resolves this issue by adding a new filename argument to translate_document() method.

@daniel-jones-deepl
Copy link
Member

Hi @seratch, thank you for finding this mistake. I have to check whether we're able to accept this Pull Request.

@daniel-jones-deepl
Copy link
Member

I'd just like to mention a workaround: (although you have probably already figured this out)
You can use translate_document_upload directly - this function is public and we will maintain backward compatibility for it.

@seratch
Copy link
Contributor Author

seratch commented Apr 6, 2022

@daniel-jones-deepl Thanks for your response! Yes, I went with the workaround: https://github.com/seratch/deepl-document-translator-for-slack/blob/b5a62d1260b91786dd7aa8d669a4a713da4127c1/app.py#L189-L252

@seratch
Copy link
Contributor Author

seratch commented May 18, 2022

@daniel-jones-deepl I've resolved the conflicts. Can you check it again if you are still interested in the change here? If not, please feel free to close this PR 👋

@daniel-jones-deepl
Copy link
Member

Hi @seratch, thanks for resolving the conflicts. Yes we want to accept this change, we are working out issues surrounding accepting contributions.

@daniel-jones-deepl
Copy link
Member

Hi @seratch, we’ve updated our contributing guidelines, so we can now accept Pull Requests. Do you agree your PR is licensed under the same license as the project: MIT License? If so I’ll accept the PR.

@seratch
Copy link
Contributor Author

seratch commented Jun 9, 2022

@daniel-jones-deepl Thanks, I don’t have any issues withe the license 👍

@daniel-jones-deepl daniel-jones-deepl merged commit 6ee107f into DeepLcom:main Jun 10, 2022
@seratch seratch deleted the filename-to-docuemtn-translation branch June 10, 2022 06:57
@daniel-jones-deepl
Copy link
Member

Thank you 😄 . I'll publish a new version in a moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@seratch @daniel-jones-deepl