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

[translation] support translation kwargs for single translation overload #19851

Merged
merged 5 commits into from
Jul 22, 2021

Conversation

@kristapratico
Copy link
Member Author

/azp run python - translation - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kristapratico kristapratico marked this pull request as ready for review July 22, 2021 15:37
:keyword str category_id: Category / custom model ID for using custom translation.
:keyword glossaries: Glossaries to apply to translation.
:paramtype glossaries: list[~azure.ai.translation.document.TranslationGlossary]
:keyword str source_storage_source: Source storage source. Default value: "AzureBlob".
Copy link
Contributor

Choose a reason for hiding this comment

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

Slightly weird how you have "Default value is "AzureBlob". Only "AzureBlob" supported"

I think for now you can just have the "Only AzureBlob supported" for now, and once more values get added, switch it to "Default value is AzureBlob"

Copy link
Member

Choose a reason for hiding this comment

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

curious, why is Python exposing this property if the user can't set other values right now?
in .NET we decided to hide it, and whenever there are other options we will expose it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was on the fence about it and Johan didn't have a preference for exposing it. Seeing as .NET is hiding it, I will hide as well. Thanks!

prefix = kwargs.pop("prefix", None)
suffix = kwargs.pop("suffix", None)
storage_type = kwargs.pop("storage_type", None)
source_storage_source = kwargs.pop("source_storage_source", None)
Copy link
Contributor

Choose a reason for hiding this comment

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

just to make sure: are you getting the "AzureBlob" default from the generated code / service, and that's why you can default to None here?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes service defaults to AzureBlob

:keyword str category_id: Category / custom model ID for using custom translation.
:keyword glossaries: Glossaries to apply to translation.
:paramtype glossaries: list[~azure.ai.translation.document.TranslationGlossary]
:keyword str source_storage_source: Source storage source. Default value: "AzureBlob".
Copy link
Member

Choose a reason for hiding this comment

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

curious, why is Python exposing this property if the user can't set other values right now?
in .NET we decided to hide it, and whenever there are other options we will expose it.

@kristapratico kristapratico merged commit e41751e into Azure:main Jul 22, 2021
@kristapratico kristapratico deleted the dt-kwargs branch July 22, 2021 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[translation] support translation kwargs in single overloaded version of begin_translation
3 participants