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.

Sign up for GitHub

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

handle duplicate types #45

Merged
merged 4 commits into from
Jan 22, 2024
Merged

handle duplicate types #45

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1abd45d
handle duplicate types
achim-k Jan 15, 2024
b1ed034
bump @foxglove/message-definition to 0.3.1
achim-k Jan 22, 2024
28502b8
remove duplicates with deep comparison
achim-k Jan 22, 2024
1f75691
add note about mcap convert bug
achim-k Jan 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
"node": ">= 14"
},
"dependencies": {
"@foxglove/message-definition": "^0.2.0",
"@foxglove/message-definition": "^0.3.1",
"md5-typescript": "^1.0.5"
},
"devDependencies": {
Expand Down
67 changes: 67 additions & 0 deletions src/parse.ros1.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -623,4 +623,71 @@ describe("fixupTypes", () => {
},
]);
});

it("handles duplicate types", () => {
const messageDefinition = `
foo_msgs/TypeA a
foo_msgs/TypeB b
================================================================================
MSG: foo_msgs/TypeA

uint64 u
================================================================================
MSG: foo_msgs/TypeB

foo_msgs/TypeA a
int32 i
================================================================================
MSG: foo_msgs/TypeA

uint64 u
`;
const types = parse(messageDefinition, { ros2: true });
expect(types).toEqual([
{
definitions: [
{
type: "foo_msgs/TypeA",
isArray: false,
name: "a",
isComplex: true,
},
{
type: "foo_msgs/TypeB",
isArray: false,
name: "b",
isComplex: true,
},
],
},
{
name: "foo_msgs/TypeA",
definitions: [
{
type: "uint64",
isArray: false,
name: "u",
isComplex: false,
},
],
},
{
name: "foo_msgs/TypeB",
definitions: [
{
type: "foo_msgs/TypeA",
isArray: false,
name: "a",
isComplex: true,
},
{
type: "int32",
isArray: false,
name: "i",
isComplex: false,
},
],
},
]);
});
});
67 changes: 67 additions & 0 deletions src/parse.ros2.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1109,4 +1109,71 @@ string<=10[<=5] up_to_five_strings_up_to_ten_characters_each
},
]);
});

it("handles duplicate types", () => {
const messageDefinition = `
foo_msgs/msg/TypeA a
foo_msgs/msg/TypeB b
================================================================================
MSG: foo_msgs/msg/TypeA

uint64 u
================================================================================
MSG: foo_msgs/msg/TypeB

foo_msgs/msg/TypeA a
int32 i
================================================================================
MSG: foo_msgs/msg/TypeA

uint64 u
`;
const types = parse(messageDefinition, { ros2: true });
expect(types).toEqual([
{
definitions: [
{
type: "foo_msgs/msg/TypeA",
isArray: false,
name: "a",
isComplex: true,
},
{
type: "foo_msgs/msg/TypeB",
isArray: false,
name: "b",
isComplex: true,
},
],
},
{
name: "foo_msgs/msg/TypeA",
definitions: [
{
type: "uint64",
isArray: false,
name: "u",
isComplex: false,
},
],
},
{
name: "foo_msgs/msg/TypeB",
definitions: [
{
type: "foo_msgs/msg/TypeA",
isArray: false,
name: "a",
isComplex: true,
},
{
type: "int32",
isArray: false,
name: "i",
isComplex: false,
},
],
},
]);
});
});
19 changes: 16 additions & 3 deletions src/parse.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@
// found at http://www.apache.org/licenses/LICENSE-2.0
// You may not use this file except in compliance with the License.

import { MessageDefinition, MessageDefinitionField } from "@foxglove/message-definition";
import {
MessageDefinition,
MessageDefinitionField,
isMsgDefEqual,
} from "@foxglove/message-definition";
import { Grammar, Parser } from "nearley";

import { buildRos2Type } from "./buildRos2Type";
Expand Down Expand Up @@ -76,12 +80,21 @@ export function parse(messageDefinition: string, options: ParseOptions = {}): Me
: buildType(definitionLines, ROS1_GRAMMAR),
);

// Filter out duplicate types.
// This will avoid that searching a type by name will return more than one result.
Copy link
Member

Choose a reason for hiding this comment

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

Would be helpful to add some historical context about mcap convert, to explain why we shouldn't delete this code

const seenTypes: MessageDefinition[] = [];
const uniqueTypes = types.filter((definition) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we deduping instead of making the search function able to return multiple or return the first that matches?
Should having duplicate types be a schema error?

Copy link
Member

Choose a reason for hiding this comment

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

+1 - seems weird to me that we would have duplicate types. At least it seems like we should confirm they are identical and throw an error if not (or always throw an error)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out that duplicates were erroneously produced by mcap convert, so we have to handle that case. I changed it now to a deep comparison instead of only checking the name.

return seenTypes.find((otherDefinition) => isMsgDefEqual(definition, otherDefinition))
Copy link
Contributor

Choose a reason for hiding this comment

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

is this deduplication as performant as it could be or as performant as _.uniqBy? I guess it's only run once per schema on startup so it's not that big of a deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this deduplication as performant as it could be or as performant as _.uniqBy?

Probably not. OK to add lodash as a dependency? If so then I'll change it to use `_.uniqBy

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please not add a dependency to this project for a bug fix?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I'd avoid adding the dep. Just wanted to see if we could get this down to a single iteration through the types, but it's not strictly necessary

? false
: seenTypes.push(definition); // Always evaluates to true;
});

// Fix up complex type names
if (options.skipTypeFixup !== true) {
fixupTypes(types);
fixupTypes(uniqueTypes);
}

return types;
return uniqueTypes;
}

/**
Expand Down
8 changes: 4 additions & 4 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,10 +416,10 @@
tsutils "^3"
typescript "^4"

"@foxglove/message-definition@^0.2.0":
version "0.2.0"
resolved "https://registry.yarnpkg.com/@foxglove/message-definition/-/message-definition-0.2.0.tgz#e31922e58e57d224717bcfbd20d3cbaad709dc6b"
integrity sha512-IQHIGCvBZR8GIua9nEpS+hsMF3gm1bfbrrnjG0rgtcFBWiNuKbzx4vIP8OIwDC+8wtwcFdfJhf4Vp5TPFiUUcQ==
"@foxglove/message-definition@^0.3.1":
version "0.3.1"
resolved "https://registry.yarnpkg.com/@foxglove/message-definition/-/message-definition-0.3.1.tgz#63f48e8f3de47bba8943d8bfa8021af635254f1c"
integrity sha512-nkPowiED67LjcKEC77CprkUG3XvSsFHHR9HEwWCuhnIC2wm0W57T1J+WWvteoArZ7SdGGlKzSYSRFyjQkgmITw==

"@humanwhocodes/config-array@^0.10.4":
version "0.10.4"
Expand Down
Loading