Adding Cognitive Services Data Plane Vision+Language SDK by DavidLiCIG · Pull Request #3604 · Azure/azure-sdk-for-net

DavidLiCIG · 2017-08-20T05:06:28Z

Description

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

[x ] I have read the contribution guidelines.
[ x] The pull request does not introduce breaking changes.

General Guidelines

[x ] Title of the pull request is clear and informative.
[x ] 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

[x ] Pull request includes test coverage for the included changes.

SDK Generation Guidelines

[x ] 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.
[x ] 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.
[x ] The *.csproj and AssemblyInfo.cs files have been updated with the new version of the SDK.

msftclas · 2017-08-20T05:06:32Z

@DavidLiCIG,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

shahabhijeet · 2017-08-21T05:45:30Z

@DavidLiCIG seems tests are failing on CI

shahabhijeet · 2017-08-21T05:47:04Z

...ane/Microsoft.CognitiveServices.Vision.Tests/Microsoft.CognitiveServices.Vision.Tests.csproj

@@ -0,0 +1,43 @@
+<Project Sdk="Microsoft.NET.Sdk">


@DavidLiCIG can you model your test project by looking at other tests.

arv100kri · 2017-08-31T01:03:44Z

...ognitiveServices/data-plane/Language/Microsoft.CognitiveServices.Language.Tests/BaseTests.cs

+using System.Net.Http;
+using System.Runtime.CompilerServices;
+
+namespace Microsoft.CognitiveServices.Language.Tests


Shouldn't the namespace be Microsoft.CognitiveServices.Language.TextAnalytics.Tests? To better match the namespace of the actual API?

arv100kri · 2017-08-31T01:04:51Z

...rvices/data-plane/Language/Microsoft.CognitiveServices.Language.Tests/DetectLanguageTests.cs

+                        }));
+
+                Assert.Equal("English", result.Documents[0].DetectedLanguages[0].Name);
+                Assert.Equal("en", result.Documents[0].DetectedLanguages[0].Iso6391Name);


Also worth adding an assert for the score

shahabhijeet

@DavidLiCIG travis changes have been pushed.
Please address the feedback and this PR is good to be merged

shahabhijeet · 2017-09-03T23:15:32Z

...ognitiveServices/data-plane/Language/Microsoft.CognitiveServices.Language.Tests/BaseTests.cs

@@ -0,0 +1,28 @@
+using Microsoft.CognitiveServices.Language.TextAnalytics;


@DavidLiCIG we have certain directory structure that is being used throughout the repo.

One being, if you have management and data plane the directories should be structured as
SDKs\CognitiveServices\management
SDKs\CognitiveServices\dataPlane
we rely on particular names to build and run tests during CI build.

Please change the directory structure accordingly.

shahabhijeet · 2017-09-03T23:17:41Z

...ne/Language/Microsoft.CognitiveServices.Language/Microsoft.CognitiveServices.Language.csproj

+
+  <PropertyGroup>
+    <TargetFrameworks>net452;netstandard1.4</TargetFrameworks>
+    <GeneratePackageOnBuild>True</GeneratePackageOnBuild>


@DavidLiCIG please remove GeneratePackageOnBuild, we have specific targets to build pacakges.
The target that we use is /t:CreateNugetPackage to create nuget packages, having it part of build system hampers the CI perf.

shahabhijeet · 2017-09-03T23:18:23Z

...ne/Language/Microsoft.CognitiveServices.Language/Microsoft.CognitiveServices.Language.csproj

+  <PropertyGroup>
+    <TargetFrameworks>net452;netstandard1.4</TargetFrameworks>
+    <GeneratePackageOnBuild>True</GeneratePackageOnBuild>
+    <PackageReleaseNotes>For detailed release notes, see: </PackageReleaseNotes>


< packagereleasenotes > should be included on the top PropertyGroup where you have PackageId and version defined.
move it under right propertygroup.

shahabhijeet · 2017-09-03T23:19:57Z

...ne/Language/Microsoft.CognitiveServices.Language/Microsoft.CognitiveServices.Language.csproj

+    <PackageReleaseNotes>For detailed release notes, see: </PackageReleaseNotes>
+  </PropertyGroup>
+
+  <PropertyGroup Condition=" '$(TargetFramework)'=='net452' ">


@DavidLiCIG remove line 20 to 32.
All these are already included for your projects.
Also we no longer use PORTABLE compiler constant in the entire repo. We have few traces of those constants being used in couple of RPs which are on track to be removed.

shahabhijeet · 2017-09-03T23:22:44Z

...ion/Microsoft.CognitiveServices.Vision.Tests/Microsoft.CognitiveServices.Vision.Tests.csproj

+  </ItemGroup>
+
+  <ItemGroup>
+    <None Update="TestImages\detection1.jpg">


@DavidLiCIG combine items in Itemgroup starting line 27 to be part of ItemGroup starting line 17
Please organize items in limited item groups especially if they logically belong to same category

shahabhijeet · 2017-09-03T23:23:30Z

...ta-plane/Vision/Microsoft.CognitiveServices.Vision/Microsoft.CognitiveServices.Vision.csproj

+    <PackageReleaseNotes>For detailed release notes, see: </PackageReleaseNotes>
+  </PropertyGroup>
+
+  <PropertyGroup Condition=" '$(TargetFramework)'=='net452' ">


@DavidLiCIG please format project file according to the feedback given to the language project

shahabhijeet · 2017-09-03T23:23:47Z

src/SDKs/CognitiveServices/data-plane/dirs.proj

@@ -0,0 +1,10 @@
+<Project ToolsVersion="15.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">


@DavidLiCIG this file is not needed.

…cial

…for-net into psSdkJson6

DavidLiCIG · 2017-09-08T23:00:48Z

@shahabhijeet Thanks. All feedbacks are addressed

shahabhijeet

Forgot to mention earlier.
add < PackageReleaseNotes > and add Release notes.
I would recommend to use < ![CDATA and then format your Release notes

shahabhijeet

Did not see AssemblyInfo for individual SDKs which has AssemblyVersion and AssemblyFileVersion.
If you are missing you need those files.

shahabhijeet · 2017-09-12T16:21:30Z

...ne/Language/Microsoft.CognitiveServices.Language/Microsoft.CognitiveServices.Language.csproj

+  <PropertyGroup>
+    <PackageId>Microsoft.CognitiveServices.Language</PackageId>
+    <Description>This client library provides access to the Microsoft Cognitive Services Language APIs.</Description>
+    <VersionPrefix>1.0.0</VersionPrefix>


@DavidLiCIG if this is a preview release, I still see your SDK nuget is not marked as preview
the version should 1.0.0-preview

shahabhijeet · 2017-09-12T16:22:05Z

...ne/Language/Microsoft.CognitiveServices.Language/Microsoft.CognitiveServices.Language.csproj

+    <TargetFrameworks>net452;netstandard1.4</TargetFrameworks>
+  </PropertyGroup>
+
+  <ItemGroup Condition=" '$(TargetFramework)' == 'net452' ">


@DavidLigCIG please remove this redundant ItemGroup, this is not needed

shahabhijeet · 2017-09-12T16:23:27Z

...ataPlane/Vision/Microsoft.CognitiveServices.Vision/Microsoft.CognitiveServices.Vision.csproj

+  <PropertyGroup>
+    <PackageId>Microsoft.CognitiveServices.Vision</PackageId>
+    <Description>This client library provides access to the Microsoft Cognitive Services Vision APIs.</Description>
+    <VersionPrefix>1.0.0</VersionPrefix>


@DavidLiCIG please mark this version as preview if this is a preview release. Release notes say this is preview but the version is not marked accordingly.
Developers who will be downloading from nuget will be confused what is the case, is it preview or not.

shahabhijeet · 2017-09-12T16:23:43Z

...ataPlane/Vision/Microsoft.CognitiveServices.Vision/Microsoft.CognitiveServices.Vision.csproj

+    <TargetFrameworks>net452;netstandard1.4</TargetFrameworks>
+  </PropertyGroup>
+
+  <ItemGroup Condition=" '$(TargetFramework)' == 'net452' ">


Remove the Itemgroup as it's not needed.

shahabhijeet · 2017-09-12T16:27:16Z

...ne/Language/Microsoft.CognitiveServices.Language/Microsoft.CognitiveServices.Language.csproj

+    <AssemblyName>Microsoft.CognitiveServices.Language</AssemblyName>
+    <PackageTags>Microsoft Cognitive Services;Cognitive Services;Cognitive Services SDK;Text Analytics API;Text Analytics;REST HTTP client;netcore451511</PackageTags>
+    <PackageReleaseNotes>This is a preview release of the Cognitive Services Language SDK. Included with this release is support for Text Analytics API.</PackageReleaseNotes>
+    <DocumentationFile>bin\$(Configuration)\$(TargetFramework)\$(AssemblyName).xml</DocumentationFile>


@DavidLiCIG generating documentation property is already applicable for the SDKs, this is again redundant, please remove

shahabhijeet · 2017-09-12T16:28:18Z

...ataPlane/Vision/Microsoft.CognitiveServices.Vision/Microsoft.CognitiveServices.Vision.csproj

+    <AssemblyName>Microsoft.CognitiveServices.Vision</AssemblyName>
+    <PackageTags>Microsoft Cognitive Services;Cognitive Services;Cognitive Services SDK;Face API;REST HTTP client;Face SDK;netcore451511</PackageTags>
+    <PackageReleaseNotes>This is a preview release of the Cognitive Services Vision SDK. Included with this release is support for Face API.</PackageReleaseNotes>
+    <DocumentationFile>bin\$(Configuration)\$(TargetFramework)\$(AssemblyName).xml</DocumentationFile>


@DavidLiCIG please remove redundant property

* Adding Vision SDK. * Add Text Analytics SDK. * Adding Vision * Removing key. * Removing unneeded files * Removing key. * Hooking up Cognitive SDKs with build. * Updating Swagger spec. * Fix build * Moving files around and adding sub namespace for TA. * Moving managemenet and dataplane folders into their respective folders. * Updating csproj and files based on comments. * Simplifying csprojects * Move to temp directory * Move back to dataPlane with right casing * Move to dataPlane directory * Fixing presumptive OS specific path convention. * Adding Assembly.Info for Language * Adding AssemblyInfo for VIsion * Updates based on feedback. * Adding a simple readme.md.

Adding Vision SDK.

0fd88bf

msftclas added the cla-not-required label Aug 20, 2017

shahabhijeet suggested changes Aug 21, 2017

View reviewed changes

shahabhijeet added the needs-revision label Aug 21, 2017

DavidLiCIG added 6 commits August 25, 2017 20:45

Add Text Analytics SDK.

975007a

Adding Vision

6d5299c

Removing key.

fcc2b9c

Removing unneeded files

97895d4

Removing key.

b14e4b9

Hooking up Cognitive SDKs with build.

3bd1247

DavidLiCIG changed the title ~~Adding Cognitive Services Data Plane Vision SDK.~~ Adding Cognitive Services Data Plane Vision+Language SDK Aug 30, 2017

DavidLiCIG added 2 commits August 30, 2017 12:09

Updating Swagger spec.

ed0da9a

Fix build

251006f

arv100kri approved these changes Aug 31, 2017

View reviewed changes

Moving files around and adding sub namespace for TA.

3d8b3d4

shahabhijeet suggested changes Sep 3, 2017

View reviewed changes

Merge branch 'psSdkJson6' into psSdkJson6

a3f68ec

shahabhijeet added Changes Requested and removed needs-revision labels Sep 7, 2017

DavidLiCIG added 5 commits September 8, 2017 11:43

Merge remote-tracking branch 'azure-sdk-for-net/psSdkJson6' into Offi…

0761253

…cial

Moving managemenet and dataplane folders into their respective folders.

5c220e3

Updating csproj and files based on comments.

cf8fee6

Simplifying csprojects

75427ce

Merge branch 'psSdkJson6' of https://github.com/DavidLiCIG/azure-sdk-…

f1b0d1f

…for-net into psSdkJson6

shahabhijeet suggested changes Sep 11, 2017

View reviewed changes

DavidLiCIG added 2 commits September 11, 2017 16:17

Move to temp directory

9e41f33

Move back to dataPlane with right casing

349f35d

DavidLiCIG added 2 commits September 11, 2017 16:22

Move to dataPlane directory

d394cc1

Fixing presumptive OS specific path convention.

a3dc7e9

shahabhijeet suggested changes Sep 12, 2017

View reviewed changes

DavidLiCIG added 4 commits September 12, 2017 10:20

Adding Assembly.Info for Language

e9e182b

Adding AssemblyInfo for VIsion

529c38d

Updates based on feedback.

58508d0

Adding a simple readme.md.

8ab7c50

shahabhijeet approved these changes Sep 12, 2017

View reviewed changes

shahabhijeet merged commit 9210f53 into Azure:psSdkJson6 Sep 12, 2017

		@@ -0,0 +1,28 @@
		using Microsoft.CognitiveServices.Language.TextAnalytics;

		@@ -0,0 +1,10 @@
		<Project ToolsVersion="15.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">

Conversation

DavidLiCIG commented Aug 20, 2017

Description

Uh oh!

msftclas commented Aug 20, 2017

Uh oh!

shahabhijeet commented Aug 21, 2017

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!

shahabhijeet 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!

DavidLiCIG commented Sep 8, 2017

Uh oh!

shahabhijeet left a comment

Choose a reason for hiding this comment

Uh oh!

shahabhijeet 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants