Repository navigation
feat(variables): Introduce new schema - #2065
szokeasaurusrex wants to merge 5 commits into
Conversation
9156448 to
4c9a5cd
Compare
9049059 to
c072ec3
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ae88d0a to
893355f
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 893355f. Configure here.
| /// Variables keyed by their names. | ||
| #[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] | ||
| #[serde(transparent)] | ||
| pub struct Variables(HashMap<String, Variable>); |
There was a problem hiding this comment.
Variable map serializes in random order
Low Severity
Variables stores entries in a HashMap, so serialized vars keys come out in non-deterministic order. Other serialized maps in this crate, such as Registers, use BTreeMap for stable output.
Reviewed by Cursor Bugbot for commit 893355f. Configure here.
There was a problem hiding this comment.
We should keep the BTreeMap here, that's generally what we do on the protocol level to have deterministic serialization
Introduce a new schema for sending extracted variables. The new schema is compatible with the old one, but provides ways to send more data about variables, including type and kind information. In the future, we can expand the schema to allow multiple representations for variables, e.g. some values may be represented as both arrays and pointers.
893355f to
796510c
Compare
| /// Variables keyed by their names. | ||
| #[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] | ||
| #[serde(transparent)] | ||
| pub struct Variables(HashMap<String, Variable>); |
There was a problem hiding this comment.
We should keep the BTreeMap here, that's generally what we do on the protocol level to have deterministic serialization
| pub struct Variables(HashMap<String, Variable>); | ||
|
|
||
| impl Variables { | ||
| pub fn is_empty(&self) -> bool { |
There was a problem hiding this comment.
I think depending on usage we could also impl Deref
| /// | ||
| /// Constructed via [`Value::ty`]. | ||
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| pub(crate) struct TypedValue { |
There was a problem hiding this comment.
| pub(crate) struct TypedValue { | |
| pub struct TypedValue { |
The module is already private so I wouldn't worry too much about visibility here. Generally we try to avoid pub(<specifier>)
| /// | ||
| /// Constructed via [`Value::ty`]. | ||
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| pub(crate) struct TypedValue { |
There was a problem hiding this comment.
I think we can inline TypedValue into Variable now.
This branch has not been deployed
3 participants
Introduce a new schema for sending extracted variables. The new schema is compatible with the old one, but provides ways to send more data about variables, including type and kind information.
In the future, we can expand the schema to allow multiple representations for variables, e.g. some values may be represented as both arrays and pointers.