[Rest Level Client] Ai document translation #14749

joheredi · 2021-04-06T19:31:31Z

This PR introduces 2 new packages

@azure-rest/core-client - which contains shared types and functions that will be reused across generated Rest Level Clients
@azure-rest/ai-document-translator - which is an auto generated rest level client that consumes @azure-rest/core-client. for the most part this package is composed of types.

A couple of changes included in this PR

Updated dev-tool so that samples work with the new @azure-rest namespace
Allow alternate versions for prettier and api-extractor to support newer TS features used in the Rest Level Clients such as Template Literal Types
- Talked to @deyaaeldeen and there doesn't seem to be any constraint for upgrading Prettier for the whole repo. To keep this PR scoped down to the Rest Level Client work I'll address the upgrade in a separate PR

TODO:

xirzec

This is very exciting! I'm really liking how both the codegen and the common code are much simpler.

common/config/rush/common-versions.json

sdk/core-rest/core-client/README.md

sdk/core-rest/core-client/src/clientHelpers.ts

sdk/documenttranslator/ai-document-translator/samples-dev/translateFromBlob.ts

sdk/documenttranslator/ai-document-translator/src/documentTranslationClient.ts

sdk/documenttranslator/ai-document-translator/src/models.ts

sdk/documenttranslator/ai-document-translator/package.json

deyaaeldeen

Looks great! I left a few comments.

sdk/core-rest/core-client/README.md

sdk/core-rest/core-client/review/core-client.api.md

sdk/core-rest/core-client/src/getClient.ts

sdk/core-rest/core-client/src/pathClientTypes.ts

sdk/core-rest/core-client/src/sendRequest.ts

deyaaeldeen · 2021-04-07T20:23:11Z

sdk/core-rest/core-client/src/urlHelpers.ts

+  return (
+    url
+      .toString()
+      // Remove double forward slashes


could this function be used by core-client's getRequestUrl? I guess we will have to downlevel from OperationSpec to the inputs this function expects.

sdk/documenttranslator/ai-document-translator-rest/package.json

sdk/documenttranslator/ai-document-translator-rest/review/ai-document-translator.api.md

sdk/documenttranslator/ai-document-translator-rest/test/text.spec.ts

witemple-msft

I left a handful of comments but my "request changes" review is only for the ones in the samples. The others are basically nitpicks.

common/tools/dev-tool/src/util/resolveProject.ts

sdk/core-rest/core-client/src/getClient.ts

sdk/core-rest/core-client/src/pathClientTypes.ts

witemple-msft · 2021-04-07T21:35:16Z

sdk/core-rest/core-client/src/common.ts

+ */
+export type ClientOptions = PipelineOptions & {
+  credentials?: {
+    scopes?: string | string[];


Out of curiosity what happens if someone passes something like scopes: ["scope1", "scope2 scope3"]? Do we zip them all together and send the request with "scope1 scope2 scope3"?

@azure/core-rest-pipeline's bearerTokenAuthenticationPolicy takes either a string or an array we just pass it as is. I believe at the end if it is a string "scope1" it would be sent out as single item array ["scope1"]

I wonder if we could just always send an array to make this contract simpler

sdk/core-rest/core-client/src/clientHelpers.ts

witemple-msft · 2021-04-07T21:45:28Z

sdk/documenttranslator/ai-document-translator-rest/review/ai-document-translator.api.md

+}
+
+// @public
+export interface GetOperations400Response extends HttpResponse {


Seems like the strong type of status isn't doing much here. We end up with a lot of interfaces that contain the exact same information that just have names for which status code they are returning. Seems like an enum use case if what we want is to strictly specify the set of possible status values for errors.

Yeah, this is a great point. I'll try to collapse these identical interface into a single one. I'll have to get this working in the generator first. I may not be able to fit it in this PR but I'll definitely address this when sending the Generator PR

sdk/documenttranslator/ai-document-translator-rest/review/ai-document-translator.api.md

sdk/documenttranslator/ai-document-translator-rest/samples-dev/translateFromBlob.ts

sdk/documenttranslator/ai-document-translator-rest/samples/v1/javascript/README.md

check-enforcer · 2021-04-08T00:39:00Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run js - [service] - ci

witemple-msft

Thanks for addressing the issues with the samples and all my little nitpicks. LGTM.

xirzec

Really good job on this, I'm very happy with how the client has turned out!

xirzec · 2021-04-14T17:58:09Z

sdk/core-rest/core-client/src/common.ts

+ */
+export type ClientOptions = PipelineOptions & {
+  credentials?: {
+    scopes?: string | string[];


I wonder if we could just always send an array to make this contract simpler

bterlson · 2021-04-15T00:43:04Z

sdk/core-rest/core-client/review/core-client.api.md

+
+// @public
+export interface Client {
+    path: unknown;


We know path will be a function at least, but probably no reason for this type to be pedantic.

made it Function

bterlson · 2021-04-15T00:44:13Z

sdk/core-rest/core-client/src/common.ts

+/**
+ * Represents the shape of an HttpResponse
+ */
+export type HttpResponse = {


I don't think we should have different types here.

joheredi · 2021-04-15T02:00:36Z

/azp run automation - pipeline-generation

azure-pipelines · 2021-04-15T02:00:58Z

No pipelines are associated with this pull request.

ghost · 2021-04-15T05:54:34Z

Hello @joheredi!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

@deyaaeldeen

This PR introduces 2 new packages - `@azure-rest/core-client` - which contains shared types and functions that will be reused across generated Rest Level Clients - `@azure-rest/ai-document-translator` - which is an auto generated rest level client that consumes `@azure-rest/core-client`. for the most part this package is composed of types. A couple of changes included in this PR - Updated dev-tool so that samples work with the new `@azure-rest` namespace - Allow alternate versions for `prettier` and `api-extractor` to support newer TS features used in the Rest Level Clients such as Template Literal Types - Talked to @deyaaeldeen and there doesn't seem to be any constraint for upgrading Prettier for the whole repo. To keep this PR scoped down to the Rest Level Client work I'll address the upgrade in a separate PR TODO: - [x] @azure-rest/core-client - [x] Functionality - [x] Readme - [x] Tests - [x] @azure-rest/ai-document-translator - [x] Functionality - [x] Samples - [x] Readme - [x] Tests

add apitestErrorCode doc (Azure#14749) * add apitestErrorCode doc * add armRPC doc * add roundTripInconsistentProperty doc * small fix * update * update signalR armTemplate * update yaml * new file * add testScenario with armTemplate * update doc * update readme.md * update doc * update NOTE section * add signalRCreateOrUpdate example file * udpate doc * add generate test scenario section * update doc * add serviceFacbric test scenario file * update managedClusters.yaml * update generateABasicTestScenario.md * update features doc * add run api test gif

joheredi requested review from bterlson, xirzec, ellismg and witemple-msft April 6, 2021 19:31

joheredi requested review from chradek, mikeharder, mitchdenny, praveenkuttappan and ramya-rao-a as code owners April 6, 2021 19:31

joheredi requested a review from deyaaeldeen April 6, 2021 19:31

xirzec reviewed Apr 6, 2021

View reviewed changes

deyaaeldeen reviewed Apr 7, 2021

View reviewed changes

witemple-msft requested changes Apr 7, 2021

View reviewed changes

joheredi requested a review from witemple-msft April 13, 2021 02:22

witemple-msft approved these changes Apr 14, 2021

View reviewed changes

xirzec approved these changes Apr 14, 2021

View reviewed changes

joheredi requested review from daviwil, maorleger, sadasant and schaabs as code owners April 15, 2021 00:35

bterlson approved these changes Apr 15, 2021

View reviewed changes

joheredi requested review from danieljurek and weshaggard as code owners April 15, 2021 03:13

joheredi added 4 commits April 15, 2021 05:36

Add new azure-rest/core-client package

4e1c0e5

Add document translation package

6657d88

Address PR feedback

4317c7e

Update generated code with more comments

d631890

joheredi added 24 commits April 15, 2021 05:36

Fix readme

9c42550

Add sanity test for DocumentTranslator

00c932f

Add clientHelpers unit test

5040779

Fix core-client readme

4be22bb

Fix broken links

52c37f0

remove locale from links

817b856

Add changelog

35d1982

Fix url import

9e219b0

Format and updated names

7db7c8d

Update with latest swagger

0c02f37

Add regenerate script

3846bbd

update endpoint

abd41f5

Update latest swagger

ffd60a3

Add sendRequest tests

4db984a

Add url helpers test

f77a88b

Update core-client

1a3c36f

Skip identity tracing test

a7f4155

Remove transforms

9d7011b

Make path of type Function

d73c11f

Test verify samples fix

6f53751

template condition

a1f37b9

Use succeeded directly

b408970

Use forward slash

47465d8

Revert changes to docs check

ded126b

joheredi added the auto-merge label Apr 15, 2021

ghost merged commit 6314022 into Azure:master Apr 15, 2021

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Rest Level Client] Ai document translation #14749

[Rest Level Client] Ai document translation #14749

joheredi commented Apr 6, 2021 •

edited

Loading

xirzec left a comment

deyaaeldeen left a comment

deyaaeldeen Apr 7, 2021

witemple-msft left a comment

witemple-msft Apr 7, 2021

joheredi Apr 12, 2021

xirzec Apr 14, 2021

witemple-msft Apr 7, 2021

joheredi Apr 12, 2021

check-enforcer bot commented Apr 8, 2021

witemple-msft left a comment

xirzec left a comment

xirzec Apr 14, 2021

bterlson Apr 15, 2021

joheredi Apr 15, 2021

bterlson Apr 15, 2021

joheredi commented Apr 15, 2021

azure-pipelines bot commented Apr 15, 2021

ghost commented Apr 15, 2021

[Rest Level Client] Ai document translation #14749

[Rest Level Client] Ai document translation #14749

Conversation

joheredi commented Apr 6, 2021 • edited Loading

xirzec left a comment

Choose a reason for hiding this comment

deyaaeldeen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

witemple-msft 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

check-enforcer bot commented Apr 8, 2021

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

witemple-msft left a comment

Choose a reason for hiding this comment

xirzec 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

joheredi commented Apr 15, 2021

azure-pipelines bot commented Apr 15, 2021

ghost commented Apr 15, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

joheredi commented Apr 6, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.