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-1916: Add overwrite fragment mods #2622

Merged
merged 5 commits into from
Mar 7, 2024

Conversation

JessamyT
Copy link
Collaborator

@JessamyT JessamyT commented Mar 6, 2024

@viambot viambot added the safe to build This pull request is marked safe to build from a trusted zone label Mar 6, 2024
@viambot
Copy link
Member

viambot commented Mar 6, 2024

Overall readability score: 55.58 (🟢 +0)

File Readability
configure-a-fleet.md 70.03 (🟢 +0.93)
View detailed metrics

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

File Readability FRE GF ARI CLI DCRS
configure-a-fleet.md 70.03 60.95 9.21 11.4 10.27 6.19
  🟢 +0.93 🟢 +0.2 🟢 +0.26 🔴 -0.1 🟢 +0.29 🟢 +0.06

Averages:

  Readability FRE GF ARI CLI DCRS
Average 55.58 47.45 10.65 13.22 12.08 7.61
  🟢 +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

Copy link
Contributor

@nataliajacobowitz nataliajacobowitz left a comment

Choose a reason for hiding this comment

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

This is great! LGTM. @piokasar Would be great for you to take a look too to see if this looks good and if there are any other cases you think are important to show.

@piokasar
Copy link

piokasar commented Mar 7, 2024

LGTM! Thank you for doing this!!

Copy link
Contributor

@skyleilani skyleilani left a comment

Choose a reason for hiding this comment

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

This is so clean. Really great to read, thank you!!

docs/fleet/configure-a-fleet.md Outdated Show resolved Hide resolved
@@ -78,7 +78,262 @@ For an example of adding a fragment to a machine, see the [Add a Rover Fragment
## Modify the config of a machine that uses a fragment

When you modify a fragment, those changes are pushed to all machines that use that fragment.
If you need to modify the config of just one machine that uses a fragment you can do the following:
If you need to modify the config of just one machine that uses a fragment you have two options:
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
If you need to modify the config of just one machine that uses a fragment you have two options:
If you need to modify the config of a singular machine that uses a fragment you have two options:

Suggestion for clarity

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 singular sounds a little bit more formal than necessary and I think just one makes sense--I'd like to keep as-is if that's okay!

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes complete sense!

docs/fleet/configure-a-fleet.md Outdated Show resolved Hide resolved
5. Add any mods you'd like to apply.
Click to view each example:

{{%expand "Change the name and attributes of a component" %}}
Copy link
Contributor

Choose a reason for hiding this comment

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

These are so beautiful I teared up a bit.

docs/fleet/configure-a-fleet.md Outdated Show resolved Hide resolved
docs/fleet/configure-a-fleet.md Outdated Show resolved Hide resolved
- Takes whatever the pin number for pin `b` was and assigns that value to the DIR pin.
Deletes the `pins.b` field.

_If you want to change the `name` of a component (for example, `motor1`), use `$set`, as shown in the change the name of a component example.
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
_If you want to change the `name` of a component (for example, `motor1`), use `$set`, as shown in the change the name of a component example.
_To change the `name` of a component (for example, `motor1`), use `$set`, as shown in the change the name of a component example.

Very optional suggestion for conciseness since there is more text in this section.

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'll swap the order of these two sentences--see below

Deletes the `pins.b` field.

_If you want to change the `name` of a component (for example, `motor1`), use `$set`, as shown in the change the name of a component example.
`$rename` is for changing the key, not the value._
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
`$rename` is for changing the key, not the value._
`$rename` is for changing attributes' keys, not their values._

or

Suggested change
`$rename` is for changing the key, not the value._
`$rename` is for changing an attributes key, not its value._

Do you know if in the context of Viam $rename can still operate on multiple attributes simultaneously so that you can rename multiple keys?

I thought it could be nice to make it clear/reiterate that you're talking about an attribute key and value just for clarity, what do you think 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.

Yes--if I understand your question correctly, the example here shows exactly that--two things being renamed simultaneously. I tested and it worked.

Yeah I can add that clarification. I'll also swap the order of these two italicized sentences, which I think will help with your comment above this too.

{
"fragment_id": "abcd7ef8-fa88-1234-b9a1-123z987e55aa",
"mods": [
{
Copy link

Choose a reason for hiding this comment

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

looking back it this - I think when the command is the same, it shouldn't be repeated - just have it as:


"$set": {
"components.motor1.attributes.max_rpm": 1818,
"components.motor1.attributes.pins.a": 30,
"components.motor1.attributes.board": "local"
}

Unless you were able to get it to work this way . Going to test this right now though to double check.

Copy link

Choose a reason for hiding this comment

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

Yeah I think they may have to be stacked if the command is the same within the same fragment_mods entry - there's a duplicate key error in the json and it discards the second.

Screen.Recording.2024-03-07.at.2.49.40.PM.mov

Copy link
Collaborator Author

@JessamyT JessamyT Mar 7, 2024

Choose a reason for hiding this comment

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

Oh interesting! That looks a lot cleaner. I didn't try combining them--I only tried it repeated, which did seem to work also for me I thought! I'll update them.

Copy link

Choose a reason for hiding this comment

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

oh interesting 🤔 I wonder why mine didn't that's odd

"fragment_id": "abcd7ef8-fa88-1234-b9a1-123z987e55aa",
"mods": [
{
"$set": {
Copy link

Choose a reason for hiding this comment

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

it would have to happen here as well

"fragment_id": "abcd7ef8-fa88-1234-b9a1-123z987e55aa",
"mods": [
{
"$rename": {
Copy link

Choose a reason for hiding this comment

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

stacking would have to be here too. Sorry I missed this earlier.
Also forgot to mention that the docs look really nice - thank you 😊

@viambot
Copy link
Member

viambot commented Mar 7, 2024

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

@JessamyT JessamyT merged commit 0ac496f into viamrobotics:main Mar 7, 2024
10 checks passed
@JessamyT JessamyT deleted the 1916overwritefragments branch March 7, 2024 20:48
sguequierre pushed a commit to sguequierre/docs that referenced this pull request Apr 25, 2024
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.

6 participants