Skip to content

Conversation

@cthrash
Copy link
Contributor

@cthrash cthrash commented Jul 24, 2018

  • Inconsistent argument type for /analyze?details=... between URL and stream methods
  • BingIU imagetype return values needs to be integral
  • Description maxCandidates argument needs to be integral

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

* Inconsistent argument type for `/analyze?details=...` between URL and stream methods
* BingIU imagetype return values needs to be integral
* Description `maxCandidates` argument needs to be integral
@AutorestCI
Copy link

AutorestCI commented Jul 24, 2018

Automation for azure-sdk-for-python

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-python#2978

@AutorestCI
Copy link

AutorestCI commented Jul 24, 2018

Automation for azure-sdk-for-node

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-node#2901

@AutorestCI
Copy link

AutorestCI commented Jul 24, 2018

Automation for azure-sdk-for-java

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-java#2237

@AutorestCI
Copy link

AutorestCI commented Jul 24, 2018

Automation for azure-sdk-for-ruby

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-ruby#1403

@cthrash
Copy link
Contributor Author

cthrash commented Jul 24, 2018

@areddish @yangyuan FYI

@AutorestCI
Copy link

AutorestCI commented Jul 24, 2018

Automation for azure-sdk-for-go

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-go#2344

@sergey-shandar
Copy link
Contributor

sergey-shandar commented Jul 27, 2018

@DavidLiCIG @felixwa please, review. The PR contains potential SDK and API breaking changes, it's important that we receive approval from you.

@felixwa
Copy link
Contributor

felixwa commented Jul 27, 2018

@yangyuan

],
"parameters": [
{
"name": "maxCandidates",
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

@felixwa
Copy link
Contributor

felixwa commented Jul 27, 2018

@sergey-shandar, this looks good to me. In future please include @yangyuan as reviewer for all Cognitive Services swagger file review. Thanks!

@sergey-shandar sergey-shandar merged commit 435479b into Azure:master Jul 30, 2018
cthrash added a commit to cthrash/azure-sdk-for-net that referenced this pull request Jul 31, 2018
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants