Adding Luis swagger files. by omarelhariry137 · Pull Request #1833 · Azure/azure-rest-api-specs

omarelhariry137 · 2017-10-10T15:26:01Z

This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.

PR information

The title of the PR is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
If applicable, the PR references the bug/issue that it fixes.
Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

I have read the contribution guidelines.
My spec meets the review criteria:
- The spec conforms to the Swagger 2.0 specification.
- The spec follows the guidelines described in the Swagger checklist document.
- Validation tools were run on swagger spec(s) and have all been fixed in this PR.

veronicagg · 2017-10-11T20:18:26Z

@omarelhariry137 Please take a look at CI validation errors, semantic checks are failing - https://travis-ci.org/Azure/azure-rest-api-specs/jobs/286094532#L570

veronicagg

Please take a look at CI, I'll post more comments soon.

veronicagg · 2017-10-16T18:52:58Z

@omarelhariry137 any updates on this PR fixing semantic validation failures: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/286094532 ?

omarelhariry137 · 2017-10-19T11:16:07Z

@veronicagg Yes, they're done now. Sorry, for taking so long.
I don't need to fix all the allowed failures section right ?

veronicagg · 2017-10-20T16:56:46Z

@omarelhariry137 Regarding the allowed failures section:

mode=linter PR_ONLY=true should be as clean as possible, particularly for errors. It's green right now, though I'm not sure the tool properly ran on the files, I'll investigate.
mode=model PR_ONLY=true should be green - I see errors reported https://travis-ci.org/Azure/azure-rest-api-specs/jobs/289922425#L584, please take a look.

veronicagg · 2017-10-20T17:07:01Z

@johanste @NiklasGustafsson please let me know if you'd like to review this PR, thanks!

veronicagg · 2017-10-20T22:03:04Z

@omarelhariry137 Regarding linter validation, please add a file similar to https://github.com/Azure/azure-rest-api-specs/blob/current/specification/cognitiveservices/data-plane/Face/readme.md under "Luis" folder, pointing to the files included in your PR.

veronicagg · 2017-10-20T23:32:44Z

Here are tools you can run locally (which CI is running) https://github.com/Azure/adx-documentation-pr/wiki/Azure-Swagger-Tools

veronicagg

Left a bunch of comments inline - many of them apply to all operations, I commented once, so you can review all.
I'd also recommend generating SDK code using autorest https://github.com/Azure/autorest/blob/master/docs/user/cli.md#autorest-command-line-interface-documentation so you can preview what your SDK would look like.

veronicagg · 2017-10-20T23:34:53Z

+          {
+            "name": "appId",
+            "in": "path",
+            "description": "Format - guid. The application ID",


you may want to end each description with "."

veronicagg · 2017-10-20T23:37:42Z

+        "responses": {
+          "200": {
+            "description": "A JSON object containing the predictions",
+            "examples": {


you could look into using x-ms-examples - https://github.com/Azure/azure-rest-api-specs/blob/master/documentation/x-ms-examples.md#why-x-ms-examples
It allows multiple examples and you can put the example in a separate file.

veronicagg · 2017-10-20T23:40:50Z

+        "operationId": "v2.0 Response - Get predictions from endpoint (post)",
+        "parameters": [
+          {
+            "name": "appId",


if parameters are shared among operations you could define them in global section and refer them from here.
https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#parameters-definitions-object
Make sure to include https://github.com/Azure/autorest/blob/master/docs/extensions/readme.md#x-ms-parameter-location as needed.

veronicagg · 2017-10-20T23:44:53Z

+        "responses": {
+          "200": {
+            "description": "A JSON object containing the predictions",
+            "examples": {


should there be a schema definition returned?
similar to https://github.com/Azure/azure-rest-api-specs/blob/current/specification/cognitiveservices/data-plane/Face/v1.0/Face.json#L52

veronicagg · 2017-10-20T23:49:36Z

+    "/apps/{appId}/versions/{versionId}/patterns": {
+      "post": {
+        "description": "Creates a new pattern feature.",
+        "operationId": "features - Create pattern feature",


operationID should be of a form similar to https://github.com/Azure/azure-rest-api-specs/blob/current/specification/cognitiveservices/data-plane/Face/v1.0/Face.json#L31
The string before the underscore "_" groups the operations in a class named that way (in code generation)
Something like "Features_CreatePattern"
The name is used for the generated method name.

veronicagg · 2017-10-20T23:52:23Z

+              "application/json": "83147"
+            }
+          },
+          "400": {


Are you intending to throw and exception in generated code? Please see https://github.com/Azure/azure-rest-api-specs/blob/2fb9a0b3b902335ff0b0033711c234431931ec9d/documentation/creating-swagger.md#default-response on how autorest acts on default response and negative responses

veronicagg · 2017-10-20T23:53:35Z

+    "/{appId}": {
+      "get": {
+        "description": "Gets the published endpoint predictions for the given query",
+        "operationId": "v2.0 Response - Get predictions from endpoint",


operationID should be of a form similar to https://github.com/Azure/azure-rest-api-specs/blob/current/specification/cognitiveservices/data-plane/Face/v1.0/Face.json#L31
The string before the underscore "_" groups the operations in a class named that way (in code generation)
Something like "Predictions_Get"
The name is used for the generated method name.

veronicagg · 2017-10-20T23:56:01Z

+        ],
+        "responses": {
+          "200": {
+            "description": ""


description should not be empty

veronicagg · 2017-10-20T23:57:46Z

+          }
+        ],
+        "responses": {
+          "200": {


is this a pageable operaation? see https://github.com/Azure/autorest/blob/master/docs/extensions/readme.md#x-ms-pageable

veronicagg · 2017-10-25T18:10:10Z

@omarelhariry137 any updates on the PR? The PR will get closed by EOD due to inactivity. If so, you can reopen it and tag me when you get back to it.

Adding Luis swagger files.

525a6bd

azuresdkci assigned veronicagg Oct 10, 2017

veronicagg suggested changes Oct 11, 2017

View reviewed changes

veronicagg added the DataPlane label Oct 11, 2017

Resolving semantic validation errors

097fb60

veronicagg suggested changes Oct 21, 2017

View reviewed changes

veronicagg closed this Oct 26, 2017

stevengum mentioned this pull request Nov 2, 2017

Performed changes according to feedback on Azure/azure-rest-api-specs/pull/1833 omarelhariry137/azure-rest-api-specs#1

Closed

9 tasks

Conversation

omarelhariry137 commented Oct 10, 2017 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR information

Quality of Swagger

Uh oh!

veronicagg commented Oct 11, 2017

Uh oh!

veronicagg left a comment

Choose a reason for hiding this comment

Uh oh!

veronicagg commented Oct 16, 2017

Uh oh!

omarelhariry137 commented Oct 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

veronicagg commented Oct 20, 2017

Uh oh!

veronicagg commented Oct 20, 2017

Uh oh!

veronicagg commented Oct 20, 2017

Uh oh!

veronicagg commented Oct 20, 2017

Uh oh!

veronicagg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

veronicagg commented Oct 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

omarelhariry137 commented Oct 10, 2017 •

edited by azuresdkci

Loading

omarelhariry137 commented Oct 19, 2017 •

edited

Loading