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

DOCS-908: Document part 1 of the cloud app API and move robot API #1706

Merged
merged 21 commits into from
Sep 7, 2023

Conversation

JessamyT
Copy link
Collaborator

@JessamyT JessamyT commented Aug 26, 2023

No description provided.

@viambot viambot added the safe to build This pull request is marked safe to build from a trusted zone label Aug 26, 2023
@JessamyT JessamyT changed the title DOCS-908: Document the cloud app API and move robot API DOCS-908: Document part 1 of the cloud app API and move robot API Aug 28, 2023
@JessamyT JessamyT marked this pull request as ready for review August 28, 2023 19:59
@JessamyT JessamyT requested a review from andf-viam August 28, 2023 22:10
docs/program/apis/cloud.md Outdated Show resolved Hide resolved
docs/program/apis/cloud.md Outdated Show resolved Hide resolved
@JessamyT JessamyT requested a review from mcvella August 29, 2023 16:17
docs/program/apis/cloud.md Outdated Show resolved Hide resolved
Co-authored-by: Sierra Guequierre <[email protected]>
Copy link
Collaborator

@sguequierre sguequierre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some clarification/speaking to user

docs/services/robot-service.md Outdated Show resolved Hide resolved
weight: 10
tags: ["robot state", "services"]
# SME: Cheuk
---

Robot Service constitutes a minimal set of APIs that most robots (Viam Server, [Viam Python SDK](https://python.viam.dev/) and various SDKs) should support.
Users will likely use the Robot Service as an entrypoint to interact with Viam robots and provide a way to get updates from the robot as a whole.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
... session management api, robot api, vision api, geometries/spatial integrations apis

(opt) Could link with the shortcodes here to these methods tables. excited to see how this page turns out, agree it'd be a great value add!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will leave for a different PR/ticket

docs/program/apis/cloud.md Outdated Show resolved Hide resolved
docs/program/apis/cloud.md Outdated Show resolved Hide resolved
Copy link
Contributor

@andf-viam andf-viam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with Sierra's comments, and leaving a few little language flow comments. Otherwise LGTM!

static/include/services/apis/cloud.md Outdated Show resolved Hide resolved
docs/program/apis/cloud.md Outdated Show resolved Hide resolved

## API

{{% alert title="Tip" color="tip" %}}
Copy link
Contributor

@andf-viam andf-viam Aug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, if this is required for every usage of the Cloud API, I would recommend making this a prerequisite step of some kind, rather than a skippable Tip.

The copy "To use ..." is perfect though, that's an ideal intro.

The outro text "Then, run methods on..." could then be something like:

Once you have instantiated an AppClient, you can run Cloud API methods against the cloud object according to the code examples for each method below.

Or similar. Not sure how to introduce cloud here, though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Maybe I should make it a section analogous to the "Control your ___ with..." section on component pages like https://docs.viam.com/components/base/#control-your-base-with-viams-client-sdk-libraries?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! That looks like just what we need here!!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd done it as a tip initially because it is different so didn't want it to look as though it wasn't different, but I think it'll work! will do

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh also I may have looked and seen this as a tip: https://docs.viam.com/services/vision/#api

Copy link
Contributor

@andf-viam andf-viam Aug 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh I could see that tip having a similar problem there as well: seemingly-skippable, yet required! Anyways, definitely out of scope for this PR!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, and on many other service pages as well! Though personally I think the tips are so colorful and eye-catching that it might be ok esp if we changed them to important or something. But indeed a problem for another PR!

@JessamyT JessamyT requested a review from sguequierre August 30, 2023 19:44
Copy link
Collaborator

@sguequierre sguequierre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some methods comments that I missed before

docs/program/apis/cloud.md Outdated Show resolved Hide resolved
docs/program/apis/cloud.md Outdated Show resolved Hide resolved
docs/program/apis/cloud.md Outdated Show resolved Hide resolved
docs/program/apis/cloud.md Outdated Show resolved Hide resolved
docs/program/apis/cloud.md Outdated Show resolved Hide resolved
@JessamyT JessamyT requested a review from sguequierre August 31, 2023 17:16
Co-authored-by: Sierra Guequierre <[email protected]>
Copy link
Collaborator

@sguequierre sguequierre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarifying comments for GRPC error raised suggestions

docs/program/apis/cloud.md Outdated Show resolved Hide resolved
docs/program/apis/cloud.md Outdated Show resolved Hide resolved
docs/program/apis/cloud.md Outdated Show resolved Hide resolved
@JessamyT JessamyT requested a review from sguequierre August 31, 2023 20:11
Copy link
Member

@stuqdog stuqdog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per offline convo, I only reviewed the code samples in docs/program/apis/cloud.md. A couple small comments but generally looks good! I would suggest trying all of these code samples briefly before merging just to make sure all the syntax is correct.

docs/program/apis/cloud.md Outdated Show resolved Hide resolved
docs/program/apis/cloud.md Outdated Show resolved Hide resolved
- The second (`[1]`) is a list of organization invites.

```python {class="line-numbers linkable-line-numbers"}
member_list = await cloud.list_organization_members()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest breaking up the return here to (member_list, invite_list) or something like that.

docs/program/apis/cloud.md Outdated Show resolved Hide resolved
@JessamyT JessamyT requested a review from stuqdog September 6, 2023 00:07
Copy link
Member

@stuqdog stuqdog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! did you get a chance to test the code samples at all?

docs/program/apis/cloud.md Outdated Show resolved Hide resolved
docs/program/apis/cloud.md Outdated Show resolved Hide resolved
docs/program/apis/cloud.md Outdated Show resolved Hide resolved
docs/program/apis/cloud.md Show resolved Hide resolved
docs/program/apis/cloud.md Outdated Show resolved Hide resolved
docs/program/apis/cloud.md Outdated Show resolved Hide resolved
docs/program/apis/cloud.md Outdated Show resolved Hide resolved
docs/program/apis/cloud.md Outdated Show resolved Hide resolved
@JessamyT
Copy link
Collaborator Author

JessamyT commented Sep 6, 2023

Ethan yeah, good call, I just finished testing them all. Updated connect code to have all the things including closing the connection. Found a Python docs bug and made a PR accordingly over there.

@JessamyT
Copy link
Collaborator Author

JessamyT commented Sep 6, 2023

@npentrel Sorry at first I'd thought you meant on the draft page but got it. Just added no_list true so should be good to go?

@JessamyT JessamyT mentioned this pull request Sep 6, 2023
docs/program/apis/robot.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@npentrel npentrel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % two more comments

@viambot
Copy link
Member

viambot commented Sep 7, 2023

You can view a rendered version of the docs from this PR at https://docs-test.viam.dev/414ac5dcdcad22ffd75d5f89ba0be7d86bf9d6ad/public

@JessamyT JessamyT merged commit f082876 into viamrobotics:main Sep 7, 2023
5 checks passed
@JessamyT JessamyT deleted the 908cloud branch September 7, 2023 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to build This pull request is marked safe to build from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants