-
Notifications
You must be signed in to change notification settings - Fork 226
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
Support nested @body
#2029
Support nested @body
#2029
Conversation
Changes in this PR will be published to the following url to try(check status of TypeSpec Pull Request Try It pipeline for publish status): Website: https://cadlwebsite.z1.web.core.windows.net/prs/2029/ |
8178a9a
to
ca2ae0f
Compare
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 see it ignores other properties in the nested model, is it expected
https://cadlplayground.z22.web.core.windows.net/prs/2029/?c=aW1wb3J0ICJAdHlwZXNwZWMvaHR0cCI7Cgp1c2luZyBUeXBlU3BlYy5IdHRwOwoKLy8gQHJvdXRlKCJpbXBsaWNpdCIpIG9wIMgOKGE6IHN0cmluZywgYsgLKTogdm9pZM5AZmFpbC0xxj7EDDHMO0Bib2R530HFQTLKQTIoxB4gIMpHxhBiOiB7xgogx1hjyCI7yBhkzxJ9xAfKds1zM8pzMygKx2DVXc9axFfJVA%3D%3D
in the playground you can uncomment the other fail operation and those fail as expected but not if you have
op fail3(
b: {
@body c: string;
d: string;
}
): void;
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.
Hmm, FWIW, I think @body
has had the impact already of dropping other unannotated properties on the response.
We only consider top-level parameters unannotated. We could perhaps make this an error, but it feels like then we should do the same on response.
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.
hhm yeah feels like the response here should be consistent
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 this should be an error (or at least a warning) some of the data is being lost here
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.
Trying to figure out how far back that response behavior goes, I think to the inception of @body
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.
This already being inconsistent maybe we don't need to do that in this PR though and can file an issue about it.
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.
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.
This looked good I think just need a new issue to talk about what should we do if there is extra unannoted properties and @body
for the nested case and in response
Fix #1141
This was originally intended to be supported and got broken during development when we added automatic body for unannotated properties at the same time. This is why the error message is hard to understand. This makes it work. The issue is still tagged design, but I consider it a bug fix.