Skip to content

Comments

ComputerVision: Fix arguments for /analyze and /describe#4610

Merged
dsgouda merged 1 commit intoAzure:psSdkJson6from
cthrash:cthrash/computer-vision-3.2-update
Aug 1, 2018
Merged

ComputerVision: Fix arguments for /analyze and /describe#4610
dsgouda merged 1 commit intoAzure:psSdkJson6from
cthrash:cthrash/computer-vision-3.2-update

Conversation

@cthrash
Copy link
Contributor

@cthrash cthrash commented Jul 30, 2018

Updates for PR1 and PR2.

  • The API client name was changed from ComputerVisionAPI to ComputerVisionClient, in keeping with other Azure SDKs.
  • The way the Azure region is specfied has changed. Specifically, the AzureRegion property was dropped in favor of an Endpoint property. If you were previously specifying an AzureRegion value, you should now specify Endpoint='https://{AzureRegion}.api.cognitive.microsoft.com' instead. This change ensures better global coverage.
  • Some inconsistencies in argument types were corrected. (a) maxCandidate is an integer for DescribeImage. (b) details is a array of enumerated types for AnalyzeImage. (c) isBWImg in the AnalyzeImage(color) response is a non-nullable bool. (d) clipArtType/lineDrawing in AnalyzeImage(imageType) is an integer (index) instead of a double.

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.

dsgouda
dsgouda suggested changes Jul 30, 2018
View reviewed changes
src/SDKs/_metadata/cognitiveservices_data-plane_ComputerVision.txt Outdated
Copy link
Contributor

@dsgouda dsgouda Jul 30, 2018

Choose a reason for hiding this comment

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

Please pull latest changes from psSdkJson6, run msbuild build.proj and regenerate the code

...gnitiveServices/dataPlane/Vision/ComputerVision/ComputerVision/Generated/Models/ImageType.cs Outdated
Copy link
Contributor

@dsgouda dsgouda Jul 30, 2018

Choose a reason for hiding this comment

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

Was this already an incorrect SDK which is being fixed or has something at the service end changed? Applies to other parameters/members too

Copy link
Contributor Author

@cthrash cthrash Jul 30, 2018

Choose a reason for hiding this comment

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

The service hasn't changed, we were preparing to make a version bump due to the name change (ComputerVisionAPI->ComputerVisionClient) so we're piling on with other changes we'd intended to fix. The service has not changed in a while.

...ComputerVision/ComputerVision/Microsoft.Azure.CognitiveServices.Vision.ComputerVision.csproj Outdated
Copy link
Contributor

@dsgouda dsgouda Jul 30, 2018

Choose a reason for hiding this comment

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

nit: indentation

@cthrash
Copy link
Contributor Author

cthrash commented Jul 30, 2018

@yangyuan fyi

dsgouda
dsgouda suggested changes Jul 31, 2018
View reviewed changes
Copy link
Contributor

@dsgouda dsgouda left a comment

Choose a reason for hiding this comment

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

Looks great apart from a couple of minor comments.

.../CognitiveServices/dataPlane/Vision/ComputerVision/ComputerVision/Properties/AssemblyInfo.cs Outdated
Copy link
Contributor

@dsgouda dsgouda Jul 31, 2018

Choose a reason for hiding this comment

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

AssemblyVersion remains the same, AssemblyFileVersion must be updated

Copy link
Contributor Author

@cthrash cthrash Jul 31, 2018

Choose a reason for hiding this comment

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

Remains the same meaning it shouldn't be changed?

Copy link
Contributor

@dsgouda dsgouda Jul 31, 2018

Choose a reason for hiding this comment

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

Yes, AssemblyVersion must not be changed

...ComputerVision/ComputerVision/Microsoft.Azure.CognitiveServices.Vision.ComputerVision.csproj Outdated
Copy link
Contributor

@dsgouda dsgouda Jul 31, 2018

Choose a reason for hiding this comment

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

Please bump the version number here.

@cthrash cthrash closed this Jul 31, 2018
@cthrash cthrash force-pushed the cthrash/computer-vision-3.2-update branch from 8010c9e to 3e28be5 Compare July 31, 2018 20:21
@cthrash cthrash deleted the cthrash/computer-vision-3.2-update branch July 31, 2018 20:40
@cthrash cthrash restored the cthrash/computer-vision-3.2-update branch July 31, 2018 20:40
@cthrash cthrash reopened this Jul 31, 2018
@cthrash cthrash closed this Jul 31, 2018
@cthrash cthrash force-pushed the cthrash/computer-vision-3.2-update branch from 162f3c8 to 3e28be5 Compare July 31, 2018 21:28
@cthrash
ComputerVision: Updates for v3.2
9410adf 
Updates for [PR1](Azure/azure-rest-api-specs#3479) and [PR2](Azure/azure-rest-api-specs#3486).

* The API client name was changed from ComputerVisionAPI to ComputerVisionClient, in keeping with other Azure SDKs.
* The way the Azure region is specfied has changed.  Specifically, the AzureRegion property was dropped in favor of an Endpoint property.  If you were previously specifying an AzureRegion value, you should now specify Endpoint='https://{AzureRegion}.api.cognitive.microsoft.com' instead. This change ensures better global coverage.
* Some inconsistencies in argument types were corrected.  (a) maxCandidate is an integer for DescribeImage. (b) details is a array of enumerated types for AnalyzeImage. (c) isBWImg in the AnalyzeImage(color) response is a non-nullable bool. (d) clipArtType/lineDrawing in AnalyzeImage(imageType) is an integer (index) instead of a double.
@cthrash cthrash reopened this Jul 31, 2018
@cthrash
Copy link
Contributor Author

cthrash commented Aug 1, 2018

PTAL 👀

dsgouda
dsgouda reviewed Aug 1, 2018
View reviewed changes
...ComputerVision/ComputerVision/Microsoft.Azure.CognitiveServices.Vision.ComputerVision.csproj
<Description>This client library provides access to the Microsoft Cognitive Services ComputerVision APIs.</Description>
<Version>3.1.0-preview</Version>
<Version>3.2.0</Version>
<AssemblyName>Microsoft.Azure.CognitiveServices.Vision.ComputerVision</AssemblyName>
Copy link
Contributor

@dsgouda dsgouda Aug 1, 2018

Choose a reason for hiding this comment

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

Please acknowledge that this is a stable release. Not a blocker

dsgouda
dsgouda approved these changes Aug 1, 2018
View reviewed changes
Copy link
Contributor

@dsgouda dsgouda left a comment

Choose a reason for hiding this comment

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

LGTM

@dsgouda dsgouda merged commit 6f83392 into Azure:psSdkJson6 Aug 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yangyuan yangyuan yangyuan approved these changes

+1 more reviewer

@dsgouda dsgouda dsgouda approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cthrash @yangyuan @dsgouda