Dt use patch #36977

kristapratico · 2024-08-21T18:03:13Z

typespec client.tsp changes: Azure/azure-rest-api-specs#30416

…or-python into dt-2.0

...ation/azure-ai-translation-document/azure/ai/translation/document/_operations/_operations.py

azure-sdk · 2024-08-21T18:38:15Z

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-ai-translation-document

catalinaperalta

@kristapratico can you open a draft pr for your typespec changes as well? I had a few comments about the changes.

...ation/azure-ai-translation-document/azure/ai/translation/document/_operations/_operations.py

...ranslation/azure-ai-translation-document/azure/ai/translation/document/_operations/_patch.py

sdk/translation/azure-ai-translation-document/azure/ai/translation/document/_patch.py

...ranslation/azure-ai-translation-document/azure/ai/translation/document/_operations/_patch.py

sdk/translation/azure-ai-translation-document/azure/ai/translation/document/_patch.py

catalinaperalta · 2024-09-05T01:32:36Z

sdk/translation/azure-ai-translation-document/azure/ai/translation/document/_patch.py

    @distributed_trace
-    def get_document_status(self, translation_id: str, document_id: str, **kwargs: Any) -> DocumentStatus:
+    def get_document_status(  # type: ignore


Does the generated client expose this method? And if so, do we still need to patch it here?

We decided to rename the parameter id to translation_id in the GA version. I was told that with the way that id is defined in the typespec, it does not allow us to rename it in the client.tsp.

tsp: https://github.com/Azure/azure-rest-api-specs/blob/8c00db9158d76c20e2d4d88c5bf0d0e72218dbe5/specification/translation/Azure.AI.DocumentTranslation/routes.tsp#L213

The following did not work:

@@clientName(DocumentTranslationClient.getDocumentStatus::parameters.id, "translation_id", "python" );

Let me know if you know a way! Would be great to not have to patch this, too.

Ah I see...yes that is an unfortunate limitation we've been dealing with, but since I think it's more noise to patch in a client library instead of just using the @clientName decorator, I'd say just use the decorator over the typespec property inline in the code. And make sure to specify Python if it's the only language that renamed it.

annatisch · 2024-09-02T01:17:03Z

sdk/translation/azure-ai-translation-document/azure/ai/translation/document/aio/_patch.py

    @distributed_trace_async
-    async def get_translation_status(self, translation_id: str, **kwargs: Any) -> TranslationStatus:
+    async def get_translation_status(self, translation_id: str, **kwargs: Any) -> TranslationStatus:  # type: ignore


Could we specify the type suppression? I'm guessing it has something to do with the method signature differing from the inherited method.

Suggested change

async def get_translation_status(self, translation_id: str, **kwargs: Any) -> TranslationStatus: # type: ignore

async def get_translation_status(self, translation_id: str, **kwargs: Any) -> TranslationStatus: # type: ignore[...]

Your guess is correct - I will add the error type for all instances.

annatisch · 2024-09-02T01:22:11Z

sdk/translation/azure-ai-translation-document/azure/ai/translation/document/models/_enums.py

@@ -30,9 +30,9 @@ class Status(str, Enum, metaclass=CaseInsensitiveEnumMeta):
    """Succeeded"""
    FAILED = "Failed"
    """Failed"""
-    CANCELLED = "Cancelled"
+    CANCELED = "Cancelled"


Any reason we changed this? (Assuming it was public. If it's not public I guess it doesn't matter so much....)

We didn't expose this in the GA version, nor the last beta. It is public by default for the current version that leverages the codegen models.

The reason I renamed the statuses here was to have consistency throughout the library. In the GA version, we decided we were against "cancelled" with a double l (don't ask me why, I don't remember 😆) and have a conversion function that changes any double l cancel to a single l in the response.

annatisch · 2024-09-05T22:32:33Z

sdk/translation/azure-ai-translation-document/tests/test_translation_async.py

@@ -463,8 +463,8 @@ def callback(request):
            assert input.source.filter.prefix == ""
            assert input.source.filter.suffix == ".txt"
            assert input.storage_type == "File"
-            assert input.targets[0].category == "fake"
-            assert input.targets[0].glossaries[0].format == "txt"
+            assert input.targets[0].category_id == "fake"


These two attributes here are breaking API changes?

Actually it seems in the APIview diff that these two properties were always called category_id and file_format?

Sorry it's a bit confusing - we renamed a lot of things in the GA version of this library. StartTranslationDetails (the type we're accessing here) was not exposed in the GA library. We didn't expose this in the beta release following the GA either, but it was used here in a test to verify deserialization.

In this PR, we make StartTranslationDetails public, as another input option to begin_translation. To be consistent with the renaming we did in the GA library, I've renamed these two properties to match.

annatisch · 2024-09-12T00:12:32Z

sdk/translation/azure-ai-translation-document/azure/ai/translation/document/models/__init__.py

-from ._patch import __all__ as _patch_all
-from ._patch import *  # pylint: disable=unused-wildcard-import
+from ._models import DocumentTranslationFileFormat
+from ._patch import TranslationGlossary


Has this been manually edited?

annatisch · 2024-09-12T00:26:44Z

sdk/translation/azure-ai-translation-document/azure/ai/translation/document/models/_patch.py

+    """Storage Source. \"AzureBlob\""""
+
+    @overload
+    def __init__(


This overload shouldn't be needed.

annatisch · 2024-09-12T00:27:14Z

sdk/translation/azure-ai-translation-document/azure/ai/translation/document/models/_patch.py

+
+def convert_status(status, ll=False):
+    if ll is False:
+        if status == "Cancelled":


We could probably make these checks case insensitive:
if status.lower() == 'cancelled'

annatisch · 2024-09-12T00:27:52Z

sdk/translation/azure-ai-translation-document/azure/ai/translation/document/models/_patch.py

+    """Storage Source. \"AzureBlob\""""
+
+    @overload
+    def __init__(


I think this overload isn't needed.

annatisch · 2024-09-12T00:32:25Z

sdk/translation/azure-ai-translation-document/azure/ai/translation/document/models/_models.py

@@ -360,152 +508,207 @@ class StatusSummary(_model_base.Model):
    """Number of in progress. Required."""
    not_yet_started: int = rest_field(name="notYetStarted")
    """Count of not yet started. Required."""
-    cancelled: int = rest_field()
+    canceled: int = rest_field(name="cancelled")


We don't rename body parameters. Why was this changed?

kristapratico added 14 commits May 28, 2024 16:00

regen dt with typespec

7943171

regen

c33562e

update samples to show diff

3cb3ac1

Merge branch 'dt-2.0' of https://github.com/kristapratico/azure-sdk-f…

179e8fe

…or-python into dt-2.0

Merge branch 'main' into dt-2.0

1f68aee

regen with necessary patch

27a7b7b

revert samples

dbeaeff

fix docs

ba346ed

sync passing

1bd78e5

get async working

09e4b87

better diff

fed317f

better diff again

e143b8d

few fixes

ab79c64

mypy/pylint

d84949b

github-actions bot added the Cognitive - Translator label Aug 21, 2024

catalinaperalta reviewed Aug 21, 2024

View reviewed changes

...ation/azure-ai-translation-document/azure/ai/translation/document/_operations/_operations.py Show resolved Hide resolved

kristapratico added 4 commits August 21, 2024 13:48

fix ci and some apiview

86be3cc

fix patched code due to rename

92ae101

fix after rename

4a3f08b

fix docstrings and add missing patch

bb48156

hamshavathimunibyraiah self-requested a review August 29, 2024 16:18

hamshavathimunibyraiah approved these changes Aug 29, 2024

View reviewed changes

catalinaperalta reviewed Aug 30, 2024

View reviewed changes

...ation/azure-ai-translation-document/azure/ai/translation/document/_operations/_operations.py Show resolved Hide resolved

catalinaperalta reviewed Sep 5, 2024

View reviewed changes

annatisch reviewed Sep 5, 2024

View reviewed changes

feedback

a599d74

annatisch reviewed Sep 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dt use patch #36977

Dt use patch #36977

kristapratico commented Aug 21, 2024 •

edited

Loading

azure-sdk commented Aug 21, 2024

catalinaperalta left a comment

catalinaperalta Sep 5, 2024

kristapratico Sep 6, 2024

catalinaperalta Sep 10, 2024

annatisch Sep 2, 2024

kristapratico Sep 6, 2024

annatisch Sep 2, 2024

kristapratico Sep 6, 2024

annatisch Sep 5, 2024

annatisch Sep 5, 2024

kristapratico Sep 6, 2024

annatisch Sep 12, 2024

annatisch Sep 12, 2024

annatisch Sep 12, 2024

annatisch Sep 12, 2024

annatisch Sep 12, 2024

	async def get_translation_status(self, translation_id: str, **kwargs: Any) -> TranslationStatus: # type: ignore
	async def get_translation_status(self, translation_id: str, **kwargs: Any) -> TranslationStatus: # type: ignore[...]

Dt use patch #36977

Are you sure you want to change the base?

Dt use patch #36977

Conversation

kristapratico commented Aug 21, 2024 • edited Loading

azure-sdk commented Aug 21, 2024

catalinaperalta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristapratico commented Aug 21, 2024 •

edited

Loading