[APP-5754] Python GetFragmentHistory SDK changes #701

piokasar · 2024-08-02T19:19:23Z

going to re-request when checks pass, requests got added immediately

stuqdog

Generally looks good! A couple small things, and we'll want to make sure tests pass before we try to merge this.

Out of curiosity, why all the unrelated formatting changes? Was this something your editor did automatically?

Edit: oh I see you removed me from review until tests pass! Let me know if/when I should come back to look.

src/viam/app/app_client.py

piokasar · 2024-08-05T15:01:00Z

Generally looks good! A couple small things, and we'll want to make sure tests pass before we try to merge this.

Out of curiosity, why all the unrelated formatting changes? Was this something your editor did automatically?

Edit: oh I see you removed me from review until tests pass! Let me know if/when I should come back to look.

thanks for looking anyways! I will re-request shortly

stuqdog

Generally looks good! Just some small suggestions and removing unneeded files from the commit.

stuqdog · 2024-08-06T14:30:05Z

.idea/.gitignore

I think the .idea folder should probably not be checked in. Looks like it's pycharm specific? I'd suggest backing these files out and then adding .idea/ to the .gitignore, or (ideally) to your own personal .gitignore (see https://stackoverflow.com/questions/7335420/global-git-ignore)

src/viam/app/app_client.py

tests/package.json

tests/mocks/services.py

tests/test_app_client.py

piokasar · 2024-08-06T17:57:37Z

tests/mocks/services.py

@@ -1173,6 +1174,9 @@ def __init__(
        available: bool,
        location_auth: LocationAuth,
        robot_part_history: List[RobotPartHistoryEntry],
+        fragment_history: List[FragmentHistoryEntry],
+        page_token: str,


in the proto these are optional - should they be optional here as well ?

I actually think we shouldn't make this a default part of the class at all. Currently we're setting them to our sentinel values when we create the service, and then passing those same sentinel values when we call the get_fragment_history method in the test, and then confirming that they're set to the sentinel value. But, because there's two places where the setting happens, we don't have confidence that the test actually did anything.

What I'd suggest instead is just removing these as arguments or set values in the constructor, such that they don't exist in general. Then, we can leave everything else as it is such that the GetFragmentHistory method sets these values, and then we can meaningfully confirm that they're properly set in the test_get_fragment_history test.

I got rid of page_token and page_limit but I kept fragment_history because that was the return value - does that make sense ? Apologies if I'm misunderstanding.

stuqdog

Looking good! One small requested change to the MockApp class, otherwise lgtm :)

stuqdog · 2024-08-08T14:19:52Z

tests/mocks/services.py

@@ -1173,6 +1174,9 @@ def __init__(
        available: bool,
        location_auth: LocationAuth,
        robot_part_history: List[RobotPartHistoryEntry],
+        fragment_history: List[FragmentHistoryEntry],
+        page_token: str,


I actually think we shouldn't make this a default part of the class at all. Currently we're setting them to our sentinel values when we create the service, and then passing those same sentinel values when we call the get_fragment_history method in the test, and then confirming that they're set to the sentinel value. But, because there's two places where the setting happens, we don't have confidence that the test actually did anything.

What I'd suggest instead is just removing these as arguments or set values in the constructor, such that they don't exist in general. Then, we can leave everything else as it is such that the GetFragmentHistory method sets these values, and then we can meaningfully confirm that they're properly set in the test_get_fragment_history test.

stuqdog · 2024-08-08T14:20:20Z

tests/mocks/services.py

@@ -1184,6 +1188,7 @@ def __init__(
        items: List[RegistryItem],
        package_type: PackageType.ValueType,
    ):
+        self.page_token = None


per above comment, this should be deleted

I'm not sure I'm understanding "such that they don't exist in general", but I ended up keeping them but setting to default values because when GetFragmentHistory tried to access them, the class attributes did not exist which gave me errors. Then in GetFragmentHistory, they were set based on the values in the request - it seems like this addressed your concern of setting twice, but if there's a more appropriate way to do this let me know!

Curious what you mean by "tried to access them"? You should be able to just set to them (i.e., self.some_fake_value = "this is a fake value that wasn't defined prior!" should work with no issues), but if you tried to access the value with foo = self.some_other_fake_value_that_has_not_been_defined then yeah, I'd expect that to fail.

stuqdog

I still think we can remove the default definitions for page_token and page_limit but there's no major harm in including them so I totally understand if you'd rather just be done with the ticket. Otherwise, lgtm!

stuqdog · 2024-08-14T19:43:57Z

tests/mocks/services.py

+        self.page_token = ""
+        self.page_limit = 0


(minor, opt) I'm pretty sure these aren't necessary/slightly surprised to hear you say you had errors if they weren't included. You should be able to delete them, see e.g. how self.public is set in the UpdateFragment method, despite no public field ever being defined prior.

That said, it's pretty harmless to have this here so it's not a big deal if you don't want to bother figuring out how to remove them.

By access, I guess meant "set". But it must have been unrelated because I was able to get rid of those default values successfully, so thank you!
Once tests pass I will merge- thank you for your help on this!!

stuqdog · 2024-08-14T19:46:19Z

tests/mocks/services.py

@@ -1184,6 +1188,7 @@ def __init__(
        items: List[RegistryItem],
        package_type: PackageType.ValueType,
    ):
+        self.page_token = None


Curious what you mean by "tried to access them"? You should be able to just set to them (i.e., self.some_fake_value = "this is a fake value that wasn't defined prior!" should work with no issues), but if you tried to access the value with foo = self.some_other_fake_value_that_has_not_been_defined then yeah, I'd expect that to fail.

GetFragmentHistory initial add

660c2af

piokasar requested a review from a team as a code owner August 2, 2024 19:19

piokasar requested review from stuqdog and lia-viam and removed request for stuqdog and lia-viam August 2, 2024 19:19

stuqdog reviewed Aug 5, 2024

View reviewed changes

src/viam/app/app_client.py Outdated Show resolved Hide resolved

src/viam/app/app_client.py Outdated Show resolved Hide resolved

piokasar force-pushed the get-frag-history branch from d8e7932 to 3ba8aea Compare August 6, 2024 14:13

make typecheck

40a03a3

piokasar force-pushed the get-frag-history branch from 3ba8aea to 40a03a3 Compare August 6, 2024 14:16

piokasar requested a review from stuqdog August 6, 2024 14:19

stuqdog requested changes Aug 6, 2024

View reviewed changes

piokasar force-pushed the get-frag-history branch 3 times, most recently from 85b34f4 to c133d3e Compare August 6, 2024 17:28

comments

eb1676e

piokasar force-pushed the get-frag-history branch 2 times, most recently from bdb622b to 5847dd1 Compare August 6, 2024 17:41

get rid of .idea changes

a97ed18

piokasar force-pushed the get-frag-history branch from 5847dd1 to a97ed18 Compare August 6, 2024 17:49

piokasar commented Aug 6, 2024

View reviewed changes

piokasar requested a review from stuqdog August 6, 2024 17:59

stuqdog reviewed Aug 8, 2024

View reviewed changes

fix test

037dad3

piokasar force-pushed the get-frag-history branch from 19d7f21 to 037dad3 Compare August 13, 2024 19:41

viamrobotics deleted a comment from stuqdog Aug 13, 2024

fix test

9d08e36

piokasar requested a review from stuqdog August 13, 2024 19:57

stuqdog approved these changes Aug 14, 2024

View reviewed changes

remove unnecessary default vals

d22f642

piokasar merged commit 068a292 into main Aug 16, 2024
12 checks passed

piokasar deleted the get-frag-history branch August 16, 2024 14:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APP-5754] Python GetFragmentHistory SDK changes #701

[APP-5754] Python GetFragmentHistory SDK changes #701

piokasar commented Aug 2, 2024 •

edited

Loading

stuqdog left a comment •

edited

Loading

piokasar commented Aug 5, 2024

stuqdog left a comment

stuqdog Aug 6, 2024

piokasar Aug 6, 2024

stuqdog Aug 8, 2024

piokasar Aug 13, 2024

stuqdog left a comment

stuqdog Aug 8, 2024

stuqdog Aug 8, 2024

piokasar Aug 13, 2024

stuqdog Aug 14, 2024

stuqdog left a comment

stuqdog Aug 14, 2024

piokasar Aug 16, 2024

stuqdog Aug 14, 2024

[APP-5754] Python GetFragmentHistory SDK changes #701

[APP-5754] Python GetFragmentHistory SDK changes #701

Conversation

piokasar commented Aug 2, 2024 • edited Loading

stuqdog left a comment • edited Loading

Choose a reason for hiding this comment

piokasar commented Aug 5, 2024

stuqdog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuqdog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuqdog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piokasar commented Aug 2, 2024 •

edited

Loading

stuqdog left a comment •

edited

Loading