-
Notifications
You must be signed in to change notification settings - Fork 10
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
Generate documentation report #365
Conversation
These seamapi/blueprint#116, seamapi/blueprint#117 are required to fix the build |
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.
@andrii-balitskyi The report and added code sample language are excellent!!! Thanks!
@razor-x Do we need to wait to merge this PR, or will it not affect the open GitBook CRs?
docs/api/_report.md
Outdated
### Endpoints | ||
|
||
- `/acs/access_groups/add_user` | ||
- `/acs/access_groups/add_user` |
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.
Why are some of these repeated?
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 catching this! It turns out that our blueprint defines an endpoint resource for each method that the actual endpoint supports. So, in the case of /acs/access_groups/add_user
, there's one endpoint definition for PUT
and one for POST
. @razor-x, is this the intended design for the blueprint?
Anyway, I'll deduplicate the endpoints for now.
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.
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.
No, this is a bug in blueprint then. There should be one endpoint per path, the supported methods defined on the endpoint
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.
- `/thermostats/set_fan_mode` | ||
- `fan_mode`: use fan_mode_setting instead. | ||
|
||
### Extra response keys |
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.
What does this mean?
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.
Some endpoints return more than one top-level key (one that is not the response_key
). Anything in this list is something we would want to eventually remove, or for new endpoints, would indicate a mistake.
@DebbieAtSeam I'll isolate the doc changes in another PR so this one will only be the report. But I don't expect any merge conflicts for PRs that only change generated files. |
Nice work @andrii-balitskyi ! Bringing this in |
#364