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.

Sign up for GitHub

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

Jump to bottom

Improve Session Management docs #1735

Merged
merged 14 commits into from
Sep 14, 2023
Merged

Improve Session Management docs #1735

merged 14 commits into from
Sep 14, 2023

Conversation

npentrel
Copy link
Collaborator

@npentrel npentrel commented Sep 1, 2023 •
edited
Loading

  • The sessions page actually just explains session management not necessarily with the API - so I've amended the title and description
  • the current copy defines some terms multiple times or uses them before they are defined - this changes the flow.
  • The usage section had a strange access the api step which seemed to just link to the API and then tell people to do the last step - we can just change the step order.

@npentrel npentrel marked this pull request as draft September 1, 2023 17:41
@viambot viambot added the safe to build This pull request is marked safe to build from a trusted zone label Sep 1, 2023
@npentrel npentrel requested a review from sguequierre September 1, 2023 18:19
@npentrel npentrel marked this pull request as ready for review September 1, 2023 18:20
sguequierre
sguequierre suggested changes Sep 1, 2023
View reviewed changes
Copy link
Contributor

@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.

Couple phrasing requests/typos addressed otherwise looks great

docs/program/apis/sessions.md
If a resource was used by client A and then by client B, and client A disconnects, the resource will not be stopped, regardless of possibly ongoing commands initiated by client A.
{{< /alert >}}

A disconnected client will attempt to establish a new session immediately prior to the next operation it performs.
Copy link
Contributor

Choose a reason for hiding this comment

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

This part looks great! Thank you for updating this naomi!

@npentrel @sguequierre
Apply suggestions from code review
9360ab8 
Co-authored-by: Sierra Guequierre <[email protected]>
@npentrel npentrel requested a review from sguequierre September 7, 2023 21:36
@npentrel
Copy link
Collaborator Author

npentrel commented Sep 7, 2023

@sguequierre ready for re-review

@viambot
Copy link
Member

viambot commented Sep 11, 2023 •
edited
Loading

Overall readability score: 54.7 (🟢 +0)

File Readability
sessions.md 53.07 (🔴 -0.38)
View detailed metrics

🟢 - Shows an increase in readability
🔴 - Shows a decrease in readability

File Readability FRE GF ARI CLI DCRS
sessions.md 53.07 41.7 12 13.8 11.78 7.24
  🔴 -0.38 🟢 +0.2 🔴 -0.38 🟢 +0.2 🔴 -0.06 🟢 +0.01

Averages:

  Readability FRE GF ARI CLI DCRS
Average 54.7 46.13 10.87 13.33 11.82 7.76
  🟢 +0 🟢 +0 🟢 +0 🟢 +0 🟢 +0 🟢 +0
View metric targets
Metric Range Ideal score
Flesch Reading Ease 100 (very easy read) to 0 (extremely difficult read) 60
Gunning Fog 6 (very easy read) to 17 (extremely difficult read) 8 or less
Auto. Read. Index 6 (very easy read) to 14 (extremely difficult read) 8 or less
Coleman Liau Index 6 (very easy read) to 17 (extremely difficult read) 8 or less
Dale-Chall Readability 4.9 (very easy read) to 9.9 (extremely difficult read) 6.9 or less

sguequierre
sguequierre reviewed Sep 12, 2023
View reviewed changes
docs/program/apis/sessions.md

*Session management* is a presence mechanism between a client and `viam-server`.
Session management allows for safer operation of robots that physically move.
As a safety precaution, the default session management configuration ensures that a robot only moves when a client is actively connected.
Copy link
Contributor

Choose a reason for hiding this comment

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

Really like this phrasing!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thank you!

sguequierre
sguequierre approved these changes Sep 12, 2023
View reviewed changes
Copy link
Contributor

@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.

LGTM couple optional rephrasing comments

docs/program/apis/sessions.md

First, use your [`RobotClient()`](/program/apis/#robot-api) instance to access the [`SessionsClient`](https://pkg.go.dev/go.viam.com/rdk/session) within your client SDK program.
This is a [gRPC](https://grpc.io/) client that `viam-server` instantiates at robot runtime.
Find `SessionsClient` defined on [GitHub](https://github.com/viamrobotics/rdk/blob/main/robot/client/client.go).
Copy link
Contributor

Choose a reason for hiding this comment

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

Fine with taking this out but AFAIK the rdk docs do not include everything in GitHub-- some methods are not documented or are missing comments in code (like expireLoop in SessionManager)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This has just moved lower on the page! All still there :)

npentrel and others added 4 commits September 12, 2023 18:24
@npentrel @sguequierre
Update sessions.md
d464756 
Co-authored-by: Sierra Guequierre <[email protected]>
@npentrel @sguequierre
Update sessions.md
235a1eb 
Co-authored-by: Sierra Guequierre <[email protected]>
@npentrel @sguequierre
Update sessions.md
7adeccd 
Co-authored-by: Sierra Guequierre <[email protected]>
@npentrel @sguequierre
Update sessions.md
7b79790 
Co-authored-by: Sierra Guequierre <[email protected]>
@viambot
Copy link
Member

viambot commented Sep 14, 2023

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

@npentrel npentrel merged commit 2bbaa92 into main Sep 14, 2023
@npentrel npentrel deleted the wip-session-mgmt branch September 14, 2023 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sguequierre sguequierre sguequierre approved these changes

Assignees
No one assigned
Labels
safe to build This pull request is marked safe to build from a trusted zone
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@npentrel @viambot @sguequierre