-
Notifications
You must be signed in to change notification settings - Fork 5.6k
FormRecognizerReceipt - Initial Commit. #6042
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.
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
Conversation
fix paths of methods (add custom) and fix path of hosttemplate
Update FormRecognizer.json
|
@dsgouda: yes this has been through API review and got approved. @bojunehsu on this thread drove the whole process. |
|
@dsgouda: I looked into squashing the commit history for just my changes but it looked bit tricky. Any suggestions are appreciated. |
git rebase -i <first_commit_id> should work, what issues are you running into |
...fication/cognitiveservices/data-plane/FormRecognizer/preview/v1.0/FormRecognizerReceipt.json
Outdated
Show resolved
Hide resolved
| } | ||
| ], | ||
| "x-ms-parameterized-host": { | ||
| "hostTemplate": "{Endpoint}/formrecognizer/v1.0-preview", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are URLs dependent on api version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is the convention for Cognitive Services.
...fication/cognitiveservices/data-plane/FormRecognizer/preview/v1.0/FormRecognizerReceipt.json
Show resolved
Hide resolved
| ], | ||
| "responses": { | ||
| "202": { | ||
| "description": "The service has accepted the request and will start processing later. It will return 'Accepted' immediately and include an 'Operation-Location' header. Client side should further query the operation status using the URL specified in this header. The 'Operation-Location' URL will expire in 48 hours.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a long running operation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is. Due to consistency with existing Cognitive Service APIs, we are not able to adopt the latest conventions. The API review board has agreed with this design.
...fication/cognitiveservices/data-plane/FormRecognizer/preview/v1.0/FormRecognizerReceipt.json
Show resolved
Hide resolved
...fication/cognitiveservices/data-plane/FormRecognizer/preview/v1.0/FormRecognizerReceipt.json
Show resolved
Hide resolved
|
|
||
| ``` yaml $(tag) == 'release_1_0' | ||
| input-file: preview/v1.0/FormRecognizer.json | ||
| input-file: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though it's implied, it is a good idea to include ocr.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, we were hoping to only reference a subset of definitions from the CV ocr.json. But as the build validation scripts end up validating even unreferenced definitions, we are removing them. So we can add ocr.json explicitly to the list of input files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A. Why does ocr.json have unreferenced definitions?
B. Regardless of whether these definitions are used, violations must be addressed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ramparab could you address this too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
dsgouda
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
dsgouda
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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:
ARM API Review Checklist
Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs.
Please follow the link to find more details on API review process.