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

RSDK-5477 - clearer tabular data objects #185

Conversation

purplenicole730
Copy link
Member

@purplenicole730 purplenicole730 commented Oct 27, 2023

Originally, the data object just came with a metadata index and printed out unclear data. Now the data object includes the metadata for each data object. We now also call toJavaScript() to display the data, which makes things clearer.
The data now looks like this:

  {
    data {
      "Lat": demo,
      "Lng": demo,
    },
    "metadata": {
      "organizationId": "demo",
      "locationId": "demo",
      "robotName": "demo",
      "robotId": "demo",
      "partName": "demo",
      "partId": "demo",
      "componentType": "demo",
      "componentName": "demo",
      "methodName": "demo",
      "methodParametersMap": [],
      "tagsList": [],
      "mimeType": "demo"
    },
    "timeRequested": "2023-10-27T19:06:00.497Z",
    "timeReceived": "2023-10-27T19:06:00.497Z"
  },

@purplenicole730 purplenicole730 changed the base branch from main to rc-0.6.0 October 27, 2023 20:00
@purplenicole730 purplenicole730 changed the title RSDK-4439-2-add-metadata-to-data-objects RSDK-4439-2-add-metadata-to-tabdata-objects Oct 27, 2023
@purplenicole730 purplenicole730 changed the title RSDK-4439-2-add-metadata-to-tabdata-objects RSDK-5477-add-metadata-to-tabdata-objects Oct 27, 2023
@purplenicole730 purplenicole730 changed the title RSDK-5477-add-metadata-to-tabdata-objects RSDK-5477-clearer-tabdata-objects Oct 27, 2023
Copy link

@felixreichenbach felixreichenbach left a comment

Choose a reason for hiding this comment

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

Am I correct with the assumption that we transfer the metadata only once (like a list of meta data entries) parallel to the data?

Are we going to provide a merged list (metadata added to each data object) or is this up to the developer to merge accordingly?

I think both options are valuable depending on the use of the data.

@purplenicole730
Copy link
Member Author

purplenicole730 commented Oct 30, 2023

Am I correct with the assumption that we transfer the metadata only once (like a list of meta data entries) parallel to the data?

Are we going to provide a merged list (metadata added to each data object) or is this up to the developer to merge accordingly?

I think both options are valuable depending on the use of the data.

@felixreichenbach
Yes, the metadata list comes all at once. If you look at the example in the PR comment, you'll see the metadata already merged in with the array that comes out of calling tabularDataByFilter(). I have confirmed and tested that the metadata is included in the binary functions.

Are you suggesting that we allow the developers to toggle this on and off?

@felixreichenbach
Copy link

Am I correct with the assumption that we transfer the metadata only once (like a list of meta data entries) parallel to the data?
Are we going to provide a merged list (metadata added to each data object) or is this up to the developer to merge accordingly?
I think both options are valuable depending on the use of the data.

@felixreichenbach Yes, the metadata list comes all at once. If you look at the example in the PR comment, you'll see the metadata already merged in with the array that comes out of calling tabularDataByFilter(). I have confirmed and tested that the metadata is included in the binary functions.

Are you suggesting that we allow the developers to toggle this on and off?

I think most people want it merged like the example. I wouldn't add anything more for now.

Copy link
Member

@cheukt cheukt left a comment

Choose a reason for hiding this comment

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

some changes requested, also can you rename the pr to RSDK-5477 - Clearer tabular data objects

src/app/data-client.ts Outdated Show resolved Hide resolved
src/app/data-client.ts Outdated Show resolved Hide resolved
src/app/data-client.ts Outdated Show resolved Hide resolved
src/app/data-client.ts Outdated Show resolved Hide resolved
@purplenicole730 purplenicole730 changed the title RSDK-5477-clearer-tabdata-objects RSDK-5477 - clearer tabdata objects Oct 30, 2023
@purplenicole730 purplenicole730 changed the title RSDK-5477 - clearer tabdata objects RSDK-5477 - clearer tabular data objects Oct 30, 2023
Copy link
Member

@cheukt cheukt left a comment

Choose a reason for hiding this comment

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

more small nits, but looks pretty good

src/app/data-client.ts Outdated Show resolved Hide resolved
src/app/data-client.ts Outdated Show resolved Hide resolved
Copy link
Member

@cheukt cheukt left a comment

Choose a reason for hiding this comment

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

the comment needs to go above the method name

src/app/data-client.ts Outdated Show resolved Hide resolved
Copy link
Member

@cheukt cheukt left a comment

Choose a reason for hiding this comment

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

LGTM!

@purplenicole730 purplenicole730 merged commit 4479cce into viamrobotics:rc-0.6.0 Oct 31, 2023
@purplenicole730 purplenicole730 deleted the RSDK-4439-2-include-metadata-to-data-objects branch October 31, 2023 12:51
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