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

[Cogs Face] Refine the documentation for Face API's new Snapshot features. #5094

Merged
merged 6 commits into from
Jan 28, 2019
Merged

[Cogs Face] Refine the documentation for Face API's new Snapshot features. #5094

merged 6 commits into from
Jan 28, 2019

Conversation

lebronJ
Copy link
Contributor

@lebronJ lebronJ commented Jan 25, 2019 •
edited
Loading

This is to further refine the documentation for Face API's new Snapshot features, added from #5074 .

Specifically, it is to add quota information and adjust the format (add new lines) for the generated Snapshot API reference documentation.

Latest improvements:

MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.

Contribution checklist:

  • I have reviewed the documentation for the workflow.
  • Validation tools were run on swagger spec(s) and have all been fixed in this PR.
  • The OpenAPI Hub was used for checking validation status and next steps.

ARM API Review Checklist

  • Service team MUST add the "WaitForARMFeedback" label if the management plane API changes fall into one of the below categories.
  • adding/removing APIs.
  • adding/removing properties.
  • adding/removing API-version.
  • adding a new service in Azure.

Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs.

  • If you are blocked on ARM review and want to get the PR merged urgently, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.
    Please follow the link to find more details on API review process.

@lebronJ lebronJ requested a review from yangyuan as a code owner January 25, 2019 14:40
@AutorestCI AutorestCI mentioned this pull request Jan 25, 2019
Merged
@AutorestCI
Copy link

AutorestCI commented Jan 25, 2019 •
edited
Loading

Automation for azure-sdk-for-js

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

@AutorestCI AutorestCI mentioned this pull request Jan 25, 2019
Merged
@AutorestCI
Copy link

AutorestCI commented Jan 25, 2019 •
edited
Loading

Automation for azure-sdk-for-python

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

@AutorestCI AutorestCI mentioned this pull request Jan 25, 2019
Merged
@AutorestCI
Copy link

AutorestCI commented Jan 25, 2019 •
edited
Loading

Automation for azure-sdk-for-ruby

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

@AutorestCI AutorestCI mentioned this pull request Jan 25, 2019
Merged
@AutorestCI
Copy link

AutorestCI commented Jan 25, 2019 •
edited
Loading

Automation for azure-sdk-for-node

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

@AutorestCI AutorestCI mentioned this pull request Jan 25, 2019
Merged
@AutorestCI
Copy link

AutorestCI commented Jan 25, 2019 •
edited
Loading

Automation for azure-sdk-for-java

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

@AutorestCI AutorestCI mentioned this pull request Jan 25, 2019
Merged
@AutorestCI
Copy link

AutorestCI commented Jan 25, 2019 •
edited
Loading

Automation for azure-sdk-for-go

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

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

yangyuan
yangyuan approved these changes Jan 27, 2019
View reviewed changes
Copy link
Member

@yangyuan yangyuan left a comment

Choose a reason for hiding this comment

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

Suggestion:
Change 。 to .
Change notstarted to notStarted (not required)

@lebronJ
Copy link
Contributor Author

lebronJ commented Jan 27, 2019 •
edited
Loading

Suggestion:
Change 。 to .
Change notstarted to notStarted (not required)

Thanks Yuan, for the notstarted it is by design. The service does return that value.
For the 。 to ., yes this is our typo but not involved in this PR. I think this is not a blocking issue but we will modify it if this pr is not merged by tomorrow.

@yangyuan
Copy link
Member

yangyuan commented Jan 27, 2019 •
edited
Loading

@lebronJ cool,
For notstarted, I know the service is using notstarted.
But as it's an API design violation enumeration values SHOULD use lowerCamelCase https://github.com/Microsoft/api-guidelines/blob/vNext/Guidelines.md , we should have a plan to change it.
Of course it's not blocking this PR.

BTW service returns nonstarted not notstarted

@lebronJ
Copy link
Contributor Author

lebronJ commented Jan 28, 2019

@lebronJ cool,
For notstarted, I know the service is using notstarted.
But as it's an API design violation enumeration values SHOULD use lowerCamelCase https://github.com/Microsoft/api-guidelines/blob/vNext/Guidelines.md , we should have a plan to change it.
Of course it's not blocking this PR.

BTW service returns nonstarted not notstarted

Understood. But I have double checked and am sure that Face service returns notstarted not nonstarted.

@yangyuan
Copy link
Member

@lebronJ cool,
For notstarted, I know the service is using notstarted.
But as it's an API design violation enumeration values SHOULD use lowerCamelCase https://github.com/Microsoft/api-guidelines/blob/vNext/Guidelines.md , we should have a plan to change it.
Of course it's not blocking this PR.
BTW service returns nonstarted not notstarted

Understood. But I have double checked and am sure that Face service returns notstarted not nonstarted.

oh, then the value in TrainingStatus is wrong. notstarted makes more sense

@lebronJ
Copy link
Contributor Author

lebronJ commented Jan 28, 2019 •
edited
Loading

@lebronJ cool,
For notstarted, I know the service is using notstarted.
But as it's an API design violation enumeration values SHOULD use lowerCamelCase https://github.com/Microsoft/api-guidelines/blob/vNext/Guidelines.md , we should have a plan to change it.
Of course it's not blocking this PR.
BTW service returns nonstarted not notstarted

Understood. But I have double checked and am sure that Face service returns notstarted not nonstarted.

oh, then the value in TrainingStatus is wrong. notstarted makes more sense

Thanks a lot to point out this, we have not realized that bug (typo) before.
Any suggestion on fixing such kind of bug? If we rename this to notstarted, it may break customers' usage with the generated SDKs.

Besides, I would appreciate if anybody can tell the correct way to add line breaks in Swagger. (Under discussion in an email thread but not quite effective.)

Anyway, these should not block this PR. Could anyone help merge this?

@jhendrixMSFT jhendrixMSFT merged commit d34f51e into Azure:master Jan 28, 2019
@lebronJ lebronJ mentioned this pull request Jan 29, 2019
Merged
9 tasks
TalluriAnusha pushed a commit to AsrOneSdk/azure-rest-api-specs that referenced this pull request Feb 6, 2019
@lebronJ @AnuTalluri
[Cogs Face] Refine the documentation for Face API's new Snapshot feat…
9d3e6d9 
…ures. (Azure#5094)

* Add 'br' tag and add quota description.

* Modify quota description string.

* Modify quota description string(apply).

* Add period after quota description.

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

@jhendrixMSFT jhendrixMSFT jhendrixMSFT approved these changes

@yangyuan yangyuan yangyuan approved these changes

Assignees

@jhendrixMSFT jhendrixMSFT

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lebronJ @AutorestCI @azuresdkci @yangyuan @jhendrixMSFT