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

Add support for schemaEncoding field #186

Merged
merged 1 commit into from
Mar 8, 2023

Conversation

achim-k
Copy link
Collaborator

@achim-k achim-k commented Mar 8, 2023

Public-Facing Changes

  • Add support for schemaEncoding field

Description

Implements the specification update in foxglove/ws-protocol#385
Fixes #8

@achim-k achim-k requested a review from jtbandes March 8, 2023 19:32
Copy link
Member

@jtbandes jtbandes left a comment

Choose a reason for hiding this comment

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

🎉

Comment on lines +19 to +22
const auto schemaEncoding =
j.find("schemaEncoding") == j.end()
? std::optional<std::string>(std::nullopt)
: std::optional<std::string>(j["schemaEncoding"].get<std::string>());
Copy link
Member

Choose a reason for hiding this comment

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

nit

Suggested change
const auto schemaEncoding =
j.find("schemaEncoding") == j.end()
? std::optional<std::string>(std::nullopt)
: std::optional<std::string>(j["schemaEncoding"].get<std::string>());
const std::optional<std::string> schemaEncoding =
j.find("schemaEncoding") == j.end()
? std::nullopt
: j["schemaEncoding"].get<std::string>();

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also initially tried that, but CPP is rather strict and complains that the types in the branches (not sure what the correct term is) differs.

@achim-k achim-k merged commit 572c340 into main Mar 8, 2023
@achim-k achim-k deleted the achim/add_schema_encoding_support branch March 8, 2023 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Support IDL message definitions
2 participants