-
Notifications
You must be signed in to change notification settings - Fork 1
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
chore(api): round trip field id #320
Conversation
#[derive(Debug, Clone, PartialEq, Deserialize, Serialize, Eq)] | ||
pub struct NewEntityWithSpans { | ||
pub name: EntityName, | ||
pub formatted_value: String, | ||
pub field_id: FieldId, |
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.
We should also add it to NewEntityWithSpan
for the legacy cases no? And what about the non-new one, like Entity
? actually I see that Entity
doesn't support the legacy span
case either...
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.
Entity doesn't support the legacy span case either...
I think I'll address this in a separate PR
We should also add it to NewEntityWithSpan for the legacy cases no?
Yep, good spot!
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.
I think I'll address this in a separate PR
yeah that's totally fair, it's just a bit sad to see the state we left the cli in 😬 thanks for cleaning up the mess
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.
Thanks 🧹
#[derive(Debug, Clone, PartialEq, Deserialize, Serialize, Eq)] | ||
pub struct NewEntityWithSpans { | ||
pub name: EntityName, | ||
pub formatted_value: String, | ||
pub field_id: FieldId, |
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.
I think I'll address this in a separate PR
yeah that's totally fair, it's just a bit sad to see the state we left the cli in 😬 thanks for cleaning up the mess
No description provided.