-
Notifications
You must be signed in to change notification settings - Fork 60
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
[Docs] Update common schema descriptions #602
[Docs] Update common schema descriptions #602
Conversation
Signed-off-by: Archer <[email protected]>
Changes AnalysisCommit SHA: 630cdf1 API ChangesSummary
ReportThe full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/11223763063/artifacts/2026176583 API Coverage
|
Spec Test Coverage Analysis
|
Signed-off-by: Archer <[email protected]>
Waiting on our editor @natebower to review this before we merge. |
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.
@Naarcha-AWS Please see my comments and changes and tag me for a reread and approval once addressed. Thanks!
Co-authored-by: Nathan Bower <[email protected]> Signed-off-by: Naarcha-AWS <[email protected]>
Co-authored-by: Nathan Bower <[email protected]> Signed-off-by: Naarcha-AWS <[email protected]>
Signed-off-by: Archer <[email protected]>
Signed-off-by: Archer <[email protected]>
Signed-off-by: Archer <[email protected]>
Signed-off-by: Naarcha-AWS <[email protected]>
@natebower: Addressed your comments. Let me know if I missed anything. |
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.
@Naarcha-AWS Final changes
Signed-off-by: Archer <[email protected]>
Co-authored-by: Nathan Bower <[email protected]> Signed-off-by: Naarcha-AWS <[email protected]>
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.
@Naarcha-AWS LGTM!
This is now ready for maintainer review and can be merged. |
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.
Found one more "AS" capitalized.
Should we be using |-
that I think folds without a space? I am worried the extra space gets stripped accidentally. Maybe >
is the right(er) one?
spec/schemas/_common.yaml
Outdated
@@ -802,8 +802,8 @@ components: | |||
type: string | |||
StringifiedEpochTimeUnitSeconds: | |||
description: |- | |||
Some APIs will return values such as numbers also as a string (notably epoch timestamps). This behavior | |||
is used to capture this behavior while keeping the semantics of the field type. | |||
Certain APIs may return values, including numbers such as epoch timestamps, as strings. This setting captures |
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.
Is this right, or should it be |
and not |-
?
Certain APIs may return values, including numbers such as epoch timestamps, as strings. This setting captures | |
Certain APIs may return values, including numbers such as epoch timestamps, as strings. This setting captures |
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.
Looks like other multi-line descriptions use |-
, see line 200.
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.
- Make sure end of lines don't have that extra space.
- Make sure
|-
folds by adding a space?
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.
Can do. Did some additional digging into this and most OpenAPI specs use a single line |
, even for multiline descriptions. I found this article that describes formatting options for descriptions: https://www.baeldung.com/swagger-format-descriptions#yaml-vs-json-formatting
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.
Updated all |-
to |
.
Signed-off-by: Archer <[email protected]>
Co-authored-by: Daniel (dB.) Doubrovkine <[email protected]> Signed-off-by: Naarcha-AWS <[email protected]>
Signed-off-by: Archer <[email protected]>
Signed-off-by: Archer <[email protected]>
Signed-off-by: Archer <[email protected]>
Signed-off-by: Archer <[email protected]>
Signed-off-by: Archer <[email protected]>
Looks like updating the pipes caused the linter to error. This was true when trying |
Updates all common schema descriptions. Would like to have our editor look at this PR before merging, Will remove from draft mode upon his approval.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.