-
Notifications
You must be signed in to change notification settings - Fork 58
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
Update ffi to reflect proto2 syntax changes #290
Conversation
@theomonnom any idea why the eventemitter test might be failing? This PR doesn't include any changes around eventemitter 🤔 |
existing_attributes = { | ||
entry.key: entry.value for entry in req.set_local_attributes.attributes | ||
} | ||
existing_attributes.update(attributes) | ||
|
||
for key, value in existing_attributes.items(): | ||
entry = req.set_local_attributes.attributes.add() | ||
entry.key = key | ||
entry.value = value | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was the logic wrong before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it showed up as a type error where lists and dicts were mixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but this might also be as a result of how the map
type in proto get's interpreted in proto2 vs proto3 syntax, not sure
That's so weird, it is passing on main, will take a look |
@lukasIO @theomonnom I see a similar issue here as in Node - a runtime error about missing required fields when running the basic_room example:
|
When implementing the proto2 syntax I made all fields required that weren't explicitly marked as optional before. Maybe this also overdid things for the RoomOptions? Not sure if these missing fields should really be required for the FFI request, wdyt @theomonnom ? |
No description provided.