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-1088: Document more of the cloud API #1814

Merged
merged 11 commits into from
Sep 14, 2023

Conversation

JessamyT
Copy link
Collaborator

@JessamyT JessamyT commented Sep 12, 2023

Code examples have all been tested (with valid strings of course)

@viambot viambot added the safe to build This pull request is marked safe to build from a trusted zone label Sep 12, 2023
@viambot
Copy link
Member

viambot commented Sep 12, 2023

Overall readability score: 54.78 (🟢 +0.08)

File Readability
cloud.md 54.2 (🟢 +4.7)
cloud.md 74.85 (🟢 +21.3)
View detailed metrics

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

File Readability FRE GF ARI CLI DCRS
cloud.md 54.2 30.36 7.97 16.7 14.96 5.75
  🟢 +4.7 🟢 +0.2 🟢 +1.64 🔴 -1 🔴 -0.46 🟢 +1.23
cloud.md 74.85 39.09 6.62 7.2 9.87 7.02
  🟢 +21.3 🟢 +9.37 🟢 +2.65 🟢 +2.8 🟢 +3.66 🟢 +1.86

Averages:

  Readability FRE GF ARI CLI DCRS
Average 54.78 46.21 10.86 13.31 11.81 7.76
  🟢 +0.08 🟢 +0.03 🟢 +0.01 🟢 +0.01 🟢 +0.01 🟢 +0.01
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

@JessamyT JessamyT requested a review from stuqdog September 13, 2023 18:17
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 typos and clarity suggestions. Thank you for taking this on!!


### UpdateLocation

Change the name of a {{< glossary_tooltip term_id="location" text="parent location" >}} and/or assign it a new location.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it's ok to use and/or generally with current style guidance, but I can't think of a better way to phrase it 😆 So my thought is it's ok?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I had the same thought :D So I just left it as it was in the Python docs

- [(viam.proto.app.Location)](https://python.viam.dev/autoapi/viam/proto/app/index.html#viam.proto.app.Location): The newly updated location.

```python {class="line-numbers linkable-line-numbers"}
# The following line takes the location with ID "abc12abcde" and moves it
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# The following line takes the location with ID "abc12abcde" and moves it
# The following line takes the location with ID "abc12abcde" and moves it to be a sub-location of the location with ID "xyz34xxxxx"

(opt) Would put this all on one line myself bc I personally think it looks better, but highly highly optional

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 think I'll leave as-is because on narrow screens it will become quite wide. However, maybe I should split the parameters onto multiple lines as well now that I look at it...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(done splitting BTW)


```python {class="line-numbers linkable-line-numbers"}
# The following line takes the location with ID "abc12abcde" and moves it
# to be a sub-location of the location with ID "xyz34xxxxx"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# to be a sub-location of the location with ID "xyz34xxxxx"


**Parameters:**

- `location_id` [(string)](https://docs.python.org/3/library/stdtypes.html#text-sequence-type-str): ID of the location to delete.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `location_id` [(string)](https://docs.python.org/3/library/stdtypes.html#text-sequence-type-str): ID of the location to delete.
- `location_id` [(string)](https://docs.python.org/3/library/stdtypes.html#text-sequence-type-str): ID of the location.

(opt)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Possibly superfluous but leaving as-is just to make sure they get it and don't somehow think it's the parent location or some such, and because this is a very short description anyway so I'm not too worried about verbosity here.


**Returns:**

- [(viam.proto.app.Location)](https://python.viam.dev/autoapi/viam/proto/app/index.html#viam.proto.app.Location): The location corresponding to the provided ID.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- [(viam.proto.app.Location)](https://python.viam.dev/autoapi/viam/proto/app/index.html#viam.proto.app.Location): The location corresponding to the provided ID.
- [(Location)](https://python.viam.dev/autoapi/viam/proto/app/index.html#viam.proto.app.Location): The location corresponding to the provided ID.

I'm actually not super sure if this is best but generally bc they're using this package I would remove the rest of the import, as they won't really need it to understand their code and it's linked

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 docs writing wise and code sample wise, would suggest getting another approval from Andrew or Naomi


### LocationAuth

Get a location’s `LocationAuth` (location secret or secrets).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Get a location’s `LocationAuth` (location secret or secrets).
Get a location’s location secret by ID.

Sorry, I know I approved already, but I think specifying by ID in description is good and would remove parenthesis. Optional but strongly suggest

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 the LocationAuth isn't just the location secret; it also contains location ID and secret IDs. How about this

Suggested change
Get a location’s `LocationAuth` (location secret or secrets).
Get a location’s authentication information by location ID.

- None

```python {class="line-numbers linkable-line-numbers"}
# Replace string with a valid location ID
Copy link
Member

Choose a reason for hiding this comment

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

there are a lot of fake location IDs in here but I don't think this advisory comment appears in all of them. Would it make sense to either remove it here, or add it everywhere else that we have a fake location_id?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're not wrong; I should probably just remove them all

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed them all!

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.

One small thing (no need to rerequest review) otherwise lgtm!

@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/bece4aadcb1873bc7ff4a57da6bc020468d3df23/public

@JessamyT JessamyT merged commit 7d1fbed into viamrobotics:main Sep 14, 2023
@JessamyT JessamyT deleted the 1088cloud branch September 14, 2023 15:26
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.

4 participants