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.

Sign up for GitHub

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

Jump to bottom

[Azure Search] Add Cognitive Skillset Operations #4609

Merged
merged 8 commits into from
Aug 1, 2018
Merged

[Azure Search] Add Cognitive Skillset Operations #4609

merged 8 commits into from
Aug 1, 2018

Conversation

jeji1101
Copy link

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
Contributor

Can one of the admins verify this patch?

@dsgouda dsgouda mentioned this pull request Jul 30, 2018
Closed
9 tasks
@jeji1101
Copy link
Author

The corresponding swagger spec PR is: Azure/azure-rest-api-specs#3386. Thanks.

@jeji1101 jeji1101 changed the title Add Cognitive Skillset Operations [DO NOT MERGE] Add Cognitive Skillset Operations Jul 31, 2018
brjohnstmsft
brjohnstmsft reviewed Jul 31, 2018
View reviewed changes
src/SDKs/Search/DataPlane/Microsoft.Azure.Search.Service/Generated/SearchServiceClient.cs Outdated
@@ -202,7 +202,6 @@ private void Initialize()
BaseUri = "https://{searchServiceName}.{searchDnsSuffix}";
ApiVersion = "2017-11-11-Preview";
SearchDnsSuffix = "search.windows.net";
SearchServiceName = "{searchServiceName}";
Copy link
Member

Choose a reason for hiding this comment

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

Did AutoRest add these? Or were they added by hand? Generated code should never be modified by hand.

Copy link
Author

Choose a reason for hiding this comment

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

I think it was added when I tried to debug the url issue. I have regenerated the generated code and the newly generated code removes the lines. I think the current change is correct. Thanks for checking.

@brjohnstmsft
Copy link
Member

@jeji1101 The code looks ok to me, other than a few minor documentation issues. I left some comments on the Swagger PR. I'll merge this once the last Swagger issues are resolved and you get signoff from another indexer/Grok SME (Eugene or Victor).

@jeji1101
Copy link
Author

jeji1101 commented Jul 31, 2018 via email

@dsgouda dsgouda requested a review from brjohnstmsft July 31, 2018 16:39
@dsgouda
Copy link
Contributor

dsgouda commented Jul 31, 2018

@jeji1101 Please join the Azure org on github here to trigger the CI builds

vl8163264128
vl8163264128 reviewed Jul 31, 2018
View reviewed changes
...Search/DataPlane/Microsoft.Azure.Search.Service/Generated/Models/KeyPhraseExtractionSkill.cs
public KeyPhraseExtractionSkillLanguage DefaultLanguageCode { get; set; }

/// <summary>
/// Gets or sets a number indicating how many key phrases to return. If
Copy link
Contributor

@vl8163264128 vl8163264128 Jul 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

"the maximum number of" instead of "how many"

vl8163264128
vl8163264128 approved these changes Jul 31, 2018
View reviewed changes
Copy link
Contributor

@vl8163264128 vl8163264128 left a comment

Choose a reason for hiding this comment

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

models look good

@brjohnstmsft brjohnstmsft changed the title [DO NOT MERGE] Add Cognitive Skillset Operations Add Cognitive Skillset Operations Jul 31, 2018
@brjohnstmsft
Copy link
Member

brjohnstmsft commented Jul 31, 2018 •
edited
Loading

@dsgouda One of the checks didn't seem to run, so we can't merge this. Any idea how to get it to run?

@dsgouda
Copy link
Contributor

dsgouda commented Jul 31, 2018

@jeji1101 needs to join the Azure org as I pointed out to trigger the build

@jeji1101
Copy link
Author

I used the link @dsgouda provided earlier to join Azure org around one or two hours ago. Thanks.

@dsgouda
Copy link
Contributor

dsgouda commented Jul 31, 2018

@azuresdkci Test this please

@dsgouda dsgouda merged commit f01a4fa into Azure:search-preview Aug 1, 2018
@stenvix
Copy link

stenvix commented Aug 6, 2018

Hi, when I can download this update using nuget?

@brjohnstmsft brjohnstmsft changed the title Add Cognitive Skillset Operations [Azure Search] Add Cognitive Skillset Operations Aug 6, 2018
@brjohnstmsft
Copy link
Member

@stenvix This is one of two major features that we're adding to the next preview version of the Azure Search .NET SDK. The other one is support for Complex Types. We're working towards releasing the next preview SDK in September.

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

@brjohnstmsft brjohnstmsft brjohnstmsft approved these changes

@vl8163264128 vl8163264128 vl8163264128 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jeji1101 @azuresdkci @brjohnstmsft @dsgouda @stenvix @vl8163264128 @hsinyij