Skip to content

RSDK-7189: add data wrappers #603

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

Merged

Conversation

purplenicole730
Copy link
Member

No description provided.

@purplenicole730 purplenicole730 marked this pull request as ready for review May 2, 2024 16:54
@purplenicole730 purplenicole730 requested a review from a team as a code owner May 2, 2024 16:54
@purplenicole730 purplenicole730 requested review from njooma and stuqdog May 2, 2024 16:54
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.

Nice! One minor comment about the tests (no need to re-request review), otherwise LGTM!

Comment on lines +923 to +924
self.organization_id = request.organization_id
self.password = request.password
Copy link
Member

Choose a reason for hiding this comment

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

We assign these values here but never do anything with them (same for organization_id and {s|m}ql_query below). If we want to keep these around we should probably test that they're properly set in the appropriate tests. That said, typically I think of these kinds of tests (confirming a value has been set on the class object) being important for methods that don't otherwise return anything, which isn't the case here. So probably it's easiest to just get rid of these entirely.

@purplenicole730 purplenicole730 merged commit 7b34a10 into viamrobotics:main May 3, 2024
12 checks passed
@purplenicole730 purplenicole730 deleted the RSDK-7189-add-data-wrappers branch May 3, 2024 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants