Skip to content

[CognitiveServices] LUIS Programmatic SDK - Updates based on latest Spec#3946

Closed
JuanAr wants to merge 4 commits intoAzure:psSdkJson6from
southworks:cognitiveservices-luis-programmatic-sdk
Closed

[CognitiveServices] LUIS Programmatic SDK - Updates based on latest Spec#3946
JuanAr wants to merge 4 commits intoAzure:psSdkJson6from
southworks:cognitiveservices-luis-programmatic-sdk

Conversation

@JuanAr
Copy link
Copy Markdown

@JuanAr JuanAr commented Dec 13, 2017 •
edited
Loading

Updates based on latest Spec changes (Azure/azure-rest-api-specs#2083)

  • Documentation updates
  • New Operation IDs
  • Unit tests for all operations

Description

This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

  • Title of the pull request 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 more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

SDK Generation Guidelines

  • If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
  • The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as the commitid of your swagger spec or link to the swagger spec, used to generate the code.
  • The *.csproj and AssemblyInfo.cs files have been updated with the new version of the SDK.

@azuresdkci
Copy link
Copy Markdown
Contributor

azuresdkci commented Dec 13, 2017

Can one of the admins verify this patch?

2 similar comments
@azuresdkci
Copy link
Copy Markdown
Contributor

azuresdkci commented Dec 13, 2017

Can one of the admins verify this patch?

@azuresdkci
Copy link
Copy Markdown
Contributor

azuresdkci commented Dec 13, 2017

Can one of the admins verify this patch?

@JuanAr JuanAr changed the title [CognitiveServices] LUIS Runtime SDK - Updates based on latest Spec [CognitiveServices] LUIS Programmatic SDK - Updates based on latest Spec Dec 13, 2017
@azuresdkci
Copy link
Copy Markdown
Contributor

azuresdkci commented Dec 13, 2017

Can one of the admins verify this patch?

shahabhijeet
shahabhijeet suggested changes Dec 14, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@shahabhijeet shahabhijeet left a comment

Choose a reason for hiding this comment

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

@JuanAr please fix your commit history, either squash or create a new PR
Can you also do the following in the below order:

  1. Pull latest from upstream (if you have already done this good)
  2. Execute msbuild.exe build.proj from the repo root
    a. This will update the build tools in the current repo
  3. Generate your SDK
  4. Build you sdk at least one from command line of IDE
    a. Msbuild.exe build.proj /t:build /p:Scope=SDKs\CognitiveService\management
  5. Update the PR

@JuanAr JuanAr force-pushed the cognitiveservices-luis-programmatic-sdk branch from ca15de4 to 7681c61 Compare December 14, 2017 17:21
@JuanAr
Copy link
Copy Markdown
Author

JuanAr commented Dec 14, 2017

Hi @shahabhijeet,
The PR changelist was squashed and the build process tested as you requested.
Regards,

@shahabhijeet
Copy link
Copy Markdown
Contributor

shahabhijeet commented Dec 15, 2017

@DavidLiCIG can you approve this PR. Also JuanAr has to be part of Azure org on github

chrimc62
chrimc62 approved these changes Dec 19, 2017
View reviewed changes
Copy link
Copy Markdown

@chrimc62 chrimc62 left a comment

Choose a reason for hiding this comment

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

Aside from the minor nit of XML comments needing periods at the end of sentences, this looks pretty good. I like the extensive tests and the model is consistent with the API. (Eventually I would prefer URLs not based on GUIDs so everything isn't two round trips.)

Comment thread ...rvices/dataPlane/Language/LUIS-Programmatic/Customizations/ApiKeyServiceClientCredentials.cs Outdated
private readonly string subscriptionKey;

/// <summary>
/// Creates a new instance of the ApiKeyServiceClientCredentails class
Copy link
Copy Markdown

@chrimc62 chrimc62 Dec 19, 2017

Choose a reason for hiding this comment

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

Documentation standards require punctuation and sentences here and throughout the file.

Comment thread ...ognitiveServices/dataPlane/Language/LUIS-Programmatic/Customizations/Models/ErrorResponse.cs
private string code;
private string message;

public string Code
Copy link
Copy Markdown

@chrimc62 chrimc62 Dec 19, 2017

Choose a reason for hiding this comment

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

Missing documentation.

shahabhijeet
shahabhijeet suggested changes Dec 21, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@shahabhijeet shahabhijeet left a comment

Choose a reason for hiding this comment

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

Please talk to David li as I have suggested the directory structure for Language data plane SDK.
If you have any questions, please let me know.

Also please onboard using aka.ms/azuregithub
This seems you are not authorized to kick off CI jobs.
Once you get onboarded the CI job will be kicked off and once the CI passes this PR will be ready to be reviewed.

@DavidLiCIG
Copy link
Copy Markdown
Contributor

DavidLiCIG commented Dec 22, 2017

Hi @shahabhijeet, this directory structure looks fine and I agree with you on the other email with regards to modular Nuget packages. Let's get this in if it looks good.

@pcostantini pcostantini force-pushed the cognitiveservices-luis-programmatic-sdk branch from 489fb2b to 46362f5 Compare January 4, 2018 23:01
@pcostantini
Copy link
Copy Markdown
Contributor

pcostantini commented Jan 4, 2018

Hi @shahabhijeet & @DavidLiCIG ,

The directory structure was updated as discussed and latest changes from the Spec added to the Programmatic SDK.

image

@pcostantini pcostantini mentioned this pull request Jan 4, 2018
Merged
10 tasks
@shahabhijeet
Copy link
Copy Markdown
Contributor

shahabhijeet commented Jan 9, 2018

@JuanAr closing at request of Pablo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@shahabhijeet shahabhijeet shahabhijeet requested changes

@chrimc62 chrimc62 chrimc62 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@DavidLiCIG DavidLiCIG

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@JuanAr @azuresdkci @shahabhijeet @DavidLiCIG @pcostantini @chrimc62 @enzocano