-
Notifications
You must be signed in to change notification settings - Fork 149
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: add arazzo rules documentation #1697
base: main
Are you sure you want to change the base?
Conversation
|
|
Coverage report
Show new covered files 🐣
Test suite run success810 tests passing in 121 suites. Report generated by 🧪jest coverage report action from 91b04e4 |
0c39fee
to
00baee1
Compare
@@ -27,6 +27,12 @@ Errors: | |||
- [security-defined](./security-defined.md) | |||
- [spec-components-invalid-map-name](./spec-components-invalid-map-name.md) | |||
- [spec](./spec.md) | |||
- [parameters-unique](./arazzo/parameters-unique.md) |
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 seems to me that we need a different structure for rules documentation.
The current version does not diverse OAS rules from AsyncAPI and Arazzo rules.
It's easy to be confused and mix spec-specific rules without checking rule details.
cc: @lornajane , @tatomyr
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.
I agree. Maybe groups like "OpenAPI", and "Arazzo".
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.
Or separator labels.
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, definitely. I'll push some changes to this branch and try to make it easier to understand.
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.
Mmmm, I added the ruleset-templates page with all the rulesets for all the formats listed. Maybe I should have split that information onto these existing minimal and recommended documentation pages though, does anyone have an opinion?
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 to reflect only having documentation for per-API configuration and keeping the per-format information as an internal-only implementation detail for now.
sorry to give duplicate work but most of the arazzo defined rules in this pr are handled by the recently merged JSON Schema definition for Arazzo 1.0. It's currently in the 1.0.0-dev branch but is expected to be merged to main very soon. |
@jeremyfiel Thank you for your contribution, having an official JSON Schema is great and will be applied for generating types, |
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.
Thanks for creating the pages for each rule, this is a great start! I will do some copy editing (I'll just push the changes if you don't mind), and figure out how to structure the information for the different description formats, but I need a few more things from you @DmitryAnansky if possible:
- answer the open questions on the PR, I can transfer the answer to all the pages where it's something that comes up repeatedly
- add some words for each of the mermaid charts, I'm not sure what we're trying to express here
- can we say what problem each rule solves? I think if I knew that, I could do the rest of the updates and then request your review.
@@ -27,6 +27,12 @@ Errors: | |||
- [security-defined](./security-defined.md) | |||
- [spec-components-invalid-map-name](./spec-components-invalid-map-name.md) | |||
- [spec](./spec.md) | |||
- [parameters-unique](./arazzo/parameters-unique.md) |
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, definitely. I'll push some changes to this branch and try to make it easier to understand.
6d8960e
to
7124c2a
Compare
7124c2a
to
f84451f
Compare
Co-authored-by: Adam Altman <[email protected]>
…per-api/per-format sections with examples
… refactor sidebar
…description format
da1e434
to
ce5d903
Compare
Can I get some reviews on this, please? There's one open conversation about page structure but otherwise I am pretty happy with how this turned out and kudos to @DmitryAnansky for all the hard work - both on the feature and on the docs! |
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.
We're going to have to discuss this offline. I'm not in agreement to move forward with this as it is.
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.
I have quite a few issues and things to discuss.
- target: name | ||
value: 'new name' | ||
- target: name | ||
value: 'another name' |
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 example is invalid not because of duplicate targets, but because targets must be JSON Pointer or XPath. This is not.
- target: name | |
value: 'new name' | |
- target: name | |
value: 'another name' | |
- target: /name | |
value: 'new name' | |
- target: /name | |
value: 'another name' |
- target: name | ||
value: 'new name' | ||
- target: description | ||
value: 'another description' |
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.
- target: name | |
value: 'new name' | |
- target: description | |
value: 'another description' | |
- target: /name | |
value: 'new name' | |
- target: /description | |
value: 'another description' |
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.
These should be in the Arazzo folder. There shouldn't be any rules specific to Spot. Spot uses Arazzo files with some specification extensions that can be validated.
@@ -0,0 +1,69 @@ | |||
# parameters-not-in-body | |||
|
|||
Requires the `in` section inside `parameters` must not contain a `body`. |
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 rule doesn't make sense. We should fix Spot's support for parameters in body.
docs/rules/spot/version-enum.md
Outdated
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 is not a spot rule. This should be part of the Arazzo spec rule. This shouldn't be a separate rule.
Co-authored-by: Adam Altman <[email protected]>
…til further notice
What/Why/How?
Add documentation for Arazzo and AsyncAPI rules
Reference
Closes: #1691
Testing
Screenshots (optional)
Check yourself
Security