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.

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 reference page /acs/systems endpoint and routes #268

Closed
3 tasks done
razor-x opened this issue Jun 17, 2024 · 34 comments · Fixed by #271
Closed
3 tasks done

Generate reference page /acs/systems endpoint and routes #268

razor-x opened this issue Jun 17, 2024 · 34 comments · Fixed by #271
Assignees

Comments

@razor-x
Copy link
Contributor

razor-x commented Jun 17, 2024

@mastafit
Copy link
Contributor

@andrii-balitskyi Any thoughts on this one? I think we should probably agree on what should be done here

@andrii-balitskyi
Copy link
Contributor

@mastafit Let's use pages from Debbie's presentation as our standard for how we want our api reference pages to look. Then, let's figure out which sections can be generated and what data our blueprint needs to allow for that generation.

@razor-x
Copy link
Contributor Author

razor-x commented Jul 27, 2024

razor-x added a commit that referenced this issue Aug 2, 2024
@razor-x razor-x changed the title Generate reference page all routes Generate reference page /acs/systems endpoint and routes Aug 5, 2024
@razor-x razor-x self-assigned this Aug 16, 2024
@DebbieAtSeam
Copy link
Contributor

@razor-x and/or @andrii-balitskyi: https://docs.seam.co/latest/~/revisions/tPVQJnfCg6s3rbLekWCU/api/acs/systems looks awesome! The generation of useful IDs for all the anchors are excellent too!

Questions...

  • Can we add a description for the acs_system at the top?
  • What's the difference between type and format? I see "Type: array Format: list," but I'm not sure what the significance of this difference is.
  • Ideally, I think I'd prefer the order to be
    acs_system_id
    string (or Type: string and then Format: string on a separate line)
    ID of the acs_system.
  • Could Boolean be caps?
  • Should I be concerned that links to other areas of the docs (both to "human-authored" topics and to other generated topics) do not work in this version?
  • I'm assuming that the "Deprecated use something else." is just a placeholder and will be replaced with the real deprecation message in a future version. Also, it would be good to use "Deprecated. Use something else."
  • Would be neat if instances of resources/props/params/etc. in descriptions would auto-link to the corresponding item, such as acs_system_id here.
  • It looks like the headings skip H2 and, instead, use H1, H3, and H4. They should not skip heading levels.
  • I really would like to add a Properties heading (parallel to Endpoints)
  • Are the events related directly to the route, or are they related more directly to the resource? For example, it would be device.connected, not devices.connected. Consequently, should the Events section be a child of the resource, instead of at the same level?
  • I'd like us to think about the readability of the page a bit (HR usage, heading levels, etc.).
  • In the endpoint pages, the POST /acs/systems/list ⇒ { acs_systems } line is excellent!
  • Where does "List Systems" come from? We can change it if we want, right?
  • Ideally, we wouldn't need "Return Type: acs_system" because it would be more efficient just to have the return type linked in the POST /acs/systems/list ⇒ { acs_systems } line. However, I don't think that GitBook supports links within codeblocks. :--(
  • I think I'd prefer Return Type to be a heading, parallel to (and after) Request Parameters, so like...
    Request Parameters
    connected_account_id
    Type: string Required: No
    ID of the connected account by which to filter the list of returned access control systems.
    Return Type
    acs_system
  • If this ref is for the HTTP API, is it useful to include tabbed code samples in other languages, or should it just be the cURL/bash request and the JSON response? (cURL/JSON is missing from the tabs now.) Also, would it be useful to include the Seam CLI code samples in this API ref, or would the Seam CLI have its own separate generated ref?
  • Can we adjust the line breaks for the code sample so that they're more vertical and require less horizontal scrolling?
  • Could we please un-italicize Return Type, Type, Required, Format, etc.?
  • What's your opinion of adding a line break between Type and Format, between Type and Required?
  • Is it true that eventually, the goal is to add additional code samples as the last section of the topic, leaving a first very simple code sample at the top?
  • Will it be possible to specify different labels in place of Request and Response for the SDKs (vs. the HTTP API)?

@andrii-balitskyi
Copy link
Contributor

andrii-balitskyi commented Aug 26, 2024

@DebbieAtSeam

Can we add a description for the acs_system at the top?

Sure, it's meant to be there but for some reason it's not rendering. I'll look into it. Thanks for catching it!

What's the difference between type and format? I see "Type: array Format: list," but I'm not sure what the significance of this difference is.

There are more useful cases like "Type: string Format: datetime" or "Type: string Format: id", but I'm not sure on "list" one. @razor-x you and Pawel did the initial design, could provide more details about this difference?

Ideally, I think I'd prefer the order to be
acs_system_id
string (or Type: string and then Format: string on a separate line)
ID of the acs_system.

Sure, I'll adjust the template.

Could Boolean be caps?

Sure!

Should I be concerned that links to other areas of the docs (both to "human-authored" topics and to other generated topics) do not work in this version?

That's a known issue and we're still fixing the links.

I'm assuming that the "Deprecated use something else." is just a placeholder and will be replaced with the real deprecation message in a future version. Also, it would be good to use "Deprecated. Use something else."

Yeah, I'll fix the deprecation message and update the format.

Would be neat if instances of resources/props/params/etc. in descriptions would auto-link to the corresponding item, such as acs_system_id here.

Yep, great idea! I'll look into it to see how we can implement that.

It looks like the headings skip H2 and, instead, use H1, H3, and H4. They should not skip heading levels.

The template used for rendering page with acs_system resource uses H2 and H3 headings. The resource name acs_system at the top and Endpoints/Events heading are H2. Property names like acs_system_id and endpoint names like /acs/systems/get are H3.
I'm not sure about Systems at the top of the page since it's not part of the template we use for generation. Probably something that gitbook adds?

I really would like to add a Properties heading (parallel to Endpoints)

Sounds good! I'll update the template.
UPD: so resource name acs_system is H2 and "Endpoints" is H2. "Properties" should be under acs_system, right? So, it'll be H3, not parallel to "Endpoints", does that sound good? @DebbieAtSeam

Are the events related directly to the route, or are they related more directly to the resource? For example, it would be device.connected, not devices.connected. Consequently, should the Events section be a child of the resource, instead of at the same level?

Events are related more directly to the resource. Making it a child of the resource makes sense. @razor-x would love the hear your thoughts on this one.

Where does "List Systems" come from? We can change it if we want, right?

Seems like it's something that gitbook adds similar to Systems at the top of api/acs/systems page. The .md file for the endpoint defines H1 header at the top of the page saying List ACS Systems which is what we control but for some reason it's not rendered on the page.

I think I'd prefer Return Type to be a heading, parallel to (and after) Request Parameters, so like...
Request Parameters
connected_account_id
Type: string Required: No
ID of the connected account by which to filter the list of returned access control systems.
Return Type
acs_system

Sounds good!

If this ref is for the HTTP API, is it useful to include tabbed code samples in other languages, or should it just be the cURL/bash request and the JSON response? (cURL/JSON is missing from the tabs now.) Also, would it be useful to include the Seam CLI code samples in this API ref, or would the Seam CLI have its own separate generated ref?

Since we're going to have separate refs for each language, I think it makes sense to remove other code samples for the current HTTP API.
We'll update the code samples to add the cURL one.
Not sure about the Seam CLI.
@razor-x What do you think?

Can we adjust the line breaks for the code sample so that they're more vertical and require less horizontal scrolling?

Yes, code samples formatting is something we haven't looked into yet.

Could we please un-italicize Return Type, Type, Required, Format, etc.?

Sure! UPD: @DebbieAtSeam, should it be bold? Or just regular text?

What's your opinion of adding a line break between Type and Format, between Type and Required?

I think it'd look better and actually line breaks were supposed to be there as we added them in the templates but the rendered page ignored them. I'll look into it.

Is it true that eventually, the goal is to add additional code samples as the last section of the topic, leaving a first very simple code sample at the top?

Yes, here's a relevant comment from Evan. Initially, we decided to render all the samples at the top.

Will it be possible to specify different labels in place of Request and Response for the SDKs (vs. the HTTP API)?

Yes, should be pretty possible :)

@andrii-balitskyi
Copy link
Contributor

@DebbieAtSeam I did most of the requested changed and you can see them here. Also, please take a look at the questions in the comment above to unblock more changes. Thanks!

@razor-x
Copy link
Contributor Author

razor-x commented Aug 26, 2024

Would be neat if instances of resources/props/params/etc. in descriptions would auto-link to the corresponding item, such as acs_system_id here.

Yep, great idea! I'll look into it to see how we can implement that.

Created issue here: #319

What's the difference between type and format?...

Type is the underlying JSON type sent over the wire, Format is the semantic meaning. OpenAPI spec actually does this too: https://swagger.io/docs/specification/data-models/data-types/ (Our formats do not match the OpenAPI ones, but we may want to fix this upstream, it won't affect what we want to define for the docs here).

Instead of displaying both, let's always show the Format. This one is more useful and implies the type (plus the type is handled by the SDK / CLI anyway): ID is always a string, Datetime is always a string, Boolean is always a Boolean. We can just convert the format with pascalCase maybe for display?

@razor-x
Copy link
Contributor Author

razor-x commented Aug 26, 2024

Events are related more directly to the resource. Making it a child of the resource makes sense.

This is only partially true. We have events like lock.unlock_door but a lock is not a resource. For now I think Events and Endpoints are the same heading level. See also #307

@razor-x
Copy link
Contributor Author

razor-x commented Aug 26, 2024

Code samples....

I thought about this: let's leave all the languages for now on the api pages. We have not implemented the SDK pages yet, so if we remove them now we will block release, since we would be effectively removing content

@razor-x
Copy link
Contributor Author

razor-x commented Aug 27, 2024

@andrii-balitskyi I tried to answer a few more. Are there any open questions / blockers left now?

@DebbieAtSeam
Copy link
Contributor

DebbieAtSeam commented Aug 27, 2024

@andrii-balitskyi You said:

UPD: so resource name acs_system is H2 and "Endpoints" is H2. "Properties" should be under acs_system, right? So, it'll be H3, not parallel to "Endpoints", does that sound good?

Correct!

acs_system
  Properties
    acs_system_id
Endpoints

@DebbieAtSeam
Copy link
Contributor

@andrii-balitskyi We said:

Could we please un-italicize Return Type, Type, Required, Format, etc.?

Sure! UPD: @DebbieAtSeam, should it be bold? Or just regular text?

Could we please try it as regular text and see how it looks? Thanks!

@DebbieAtSeam
Copy link
Contributor

DebbieAtSeam commented Aug 27, 2024

@andrii-balitskyi Looks excellent! Thanks so much! A few picky questions and comments...
image
image
(Oops! It looks like there's an extra space, but there's not really an extra space.)
image
image
image
Again, really lovely! Very exciting!

@andrii-balitskyi
Copy link
Contributor

andrii-balitskyi commented Aug 27, 2024

Could we please try it as regular text and see how it looks? Thanks!

@DebbieAtSeam It should be in regular text already as that's how I left it yesterday.

@andrii-balitskyi
Copy link
Contributor

andrii-balitskyi commented Aug 27, 2024

@andrii-balitskyi Looks excellent! Thanks so much! A few picky questions and comments...

@DebbieAtSeam Many thanks! Oh yeah, you're right about the heading levels. For some reason, each heading defined with Markdown in our template is being rendered with an increased level by one, so ## became H3 and ### became H4 (God knows why! 😄). That's why H2 was missing. I've decreased the heading levels by one, and now they seem to be rendering without omitting any levels [link]
image

@andrii-balitskyi
Copy link
Contributor

andrii-balitskyi commented Aug 27, 2024

@DebbieAtSeam Regarding the extraneous space, I think it's there because "external_type_display_name" in the deprecation message is wrapped in backticks [ref]. As a result, "external_type_display_name" is rendered as code. You can actually see that "Use" and "external_type_display_name" are in different fonts.
image

@andrii-balitskyi
Copy link
Contributor

andrii-balitskyi commented Aug 27, 2024

image

We'll need to figure out where to store the missing description alongside the current one. If we put the missing description inside the resource description where the current one lives, we'll need to determine how to differentiate these descriptions so they can be used in the appropriate parts of the doc page.

UPD: Also, I'm not sure how we can place the missing description right under the H1 "Systems" header, which is added by GitBook based on the table of contents defined in docs/SUMMARY.md.

@andrii-balitskyi
Copy link
Contributor

andrii-balitskyi commented Aug 27, 2024

Where does "List Systems" come from? We can change it if we want, right?

It seems to come from docs/SUMMARY.md [link]. "List Systems" is defined there by this line: * [List Systems](api/acs/systems/list.md). This heading appears to become both a tab on the left-hand side navigation bar and an H1 header for api/acs/systems/list.md page. I assume we're going to generate the API Reference section of the summary as well, so we can definitely change it.

@andrii-balitskyi
Copy link
Contributor

andrii-balitskyi commented Aug 27, 2024

List of recent changes:

  • Property "Type" removed from the resource page in favor of capitalized "Format"
  • "Properties" heading added to the resource page
  • H2 heading is no longer skipped on the resource page
  • Only the first paragraph is included in the endpoint description in the "Endpoints" section
  • Resource description moved to appear after the resource name

Latest preview here.

@andrii-balitskyi
Copy link
Contributor

@andrii-balitskyi I tried to answer a few more. Are there any open questions / blockers left now?

I'm not sure how we should set up code sample formatters for each language. Do you have any ideas?

@DebbieAtSeam
Copy link
Contributor

@andrii-balitskyi You said:

Where does "List Systems" come from? We can change it if we want, right?

It seems to come from docs/SUMMARY.md [link]. "List Systems" is defined there by this line: * [List Systems](api/acs/systems/list.md). This heading appears to become both a tab on the left-hand side navigation bar and an H1 header for api/acs/systems/list.md page. I assume we're going to generate the API Reference section of the summary as well, so we can definitely change it.

I think GitBook does a combo of a manually-defined table of contents (docs/SUMMARY.md) and an automatically-generated table of contents. I'd prefer if we could be prescriptive about what ends up in the table of contents.

@DebbieAtSeam
Copy link
Contributor

@andrii-balitskyi What is an "Id"? Is this a UUID? Is this a string that needs to be a UUID? (Also, it should be "ID" or "id".)
image

@DebbieAtSeam
Copy link
Contributor

@andrii-balitskyi Should we be more specific about things that are lists? Lists of strings, etc.?
image

@DebbieAtSeam
Copy link
Contributor

@andrii-balitskyi Formatting question...
image

@DebbieAtSeam
Copy link
Contributor

@andrii-balitskyi Thanks again and again for bearing with all my picky questions! It's looking lovely!

@razor-x
Copy link
Contributor Author

razor-x commented Aug 28, 2024

@andrii-balitskyi You said:

Where does "List Systems" come from? We can change it if we want, right?

It seems to come from docs/SUMMARY.md [link]. "List Systems" is defined there by this line: * [List Systems](api/acs/systems/list.md). This heading appears to become both a tab on the left-hand side navigation bar and an H1 header for api/acs/systems/list.md page. I assume we're going to generate the API Reference section of the summary as well, so we can definitely change it.

I think GitBook does a combo of a manually-defined table of contents (docs/SUMMARY.md) and an automatically-generated table of contents. I'd prefer if we could be prescriptive about what ends up in the table of contents.

This should be handled by the work in #321 but we can review the link tiles in the nav again after that is merged and beta is updated. That section of the summary will eventually be generated.

@andrii-balitskyi
Copy link
Contributor

andrii-balitskyi commented Aug 28, 2024

@andrii-balitskyi What is an "Id"? Is this a UUID? Is this a string that needs to be a UUID? (Also, it should be "ID" or "id".)

Correct, this is a string that needs to be a UUID.
Fixed casing of 'id'. Preview here.

@andrii-balitskyi
Copy link
Contributor

@andrii-balitskyi Should we be more specific about things that are lists? Lists of strings, etc.? image

Yeah, I think that'd be more helpful. For this to be implemented, we'd need to make some changes upstream.
@razor-x, what do you think about this? Should we add something like listElementType if the property format is list?

@andrii-balitskyi
Copy link
Contributor

@andrii-balitskyi Formatting question... image

If we wrap the return type in a code block, we'll lose the ability to link the return resource to its respective page, as links within code blocks are not rendered as clickable links. I've sent a request to GitBook support to see what they can suggest about this issue.

@andrii-balitskyi
Copy link
Contributor

andrii-balitskyi commented Aug 28, 2024

UPD: Also, I'm not sure how we can place the missing description right under the H1 "Systems" header, which is added by GitBook based on the table of contents defined in docs/SUMMARY.md.

Evan and I figured out how to place that short description right under the H1 page header. Now we need to determine where to put the missing description on seam-connect alongside the existing H2 header description ("Represents an access control system" text under acs_system in the screenshot below).

@andrii-balitskyi
Copy link
Contributor

andrii-balitskyi commented Aug 28, 2024

I also decided to add enum values to enum properties. Let me know what you think. Preview here.

@razor-x
Copy link
Contributor Author

razor-x commented Aug 29, 2024

@DebbieAtSeam Let's focus on any blockers for getting this live: basically what is left to do that would make this better than what we have live now? We will continue to improve as we go.

We are working on fixing the code format and the links that are pointing to GitHub. Anything else that would block releasing this by Friday?

@andrii-balitskyi
Copy link
Contributor

@andrii-balitskyi Should we be more specific about things that are lists? Lists of strings, etc.? image

Yeah, I think that'd be more helpful. For this to be implemented, we'd need to make some changes upstream. @razor-x, what do you think about this? Should we add something like listElementType if the property format is list?

@razor-x, what do you think about this one?

@razor-x
Copy link
Contributor Author

razor-x commented Aug 29, 2024

@andrii-balitskyi We have

interface ResourceListResponse extends BaseResponse {
  responseType: 'resource_list'
  responseKey: string
  resourceType: string
}

so we should be able to do this with responseType === 'resource_list' and the resourceType (instead of listElementType).

@razor-x razor-x closed this as completed Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants