-
Notifications
You must be signed in to change notification settings - Fork 115
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
1255 Make fdc3.fileAttachment and independent type #1261
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for fdc3 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -38,38 +38,7 @@ | |||
"$ref": "action.schema.json#" |
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.
@Yannick-Malins can you change the additionalProperties.anyOf
to oneOf
- the semantics are virtually the same, but the code generation via QuickType differs (it'll merge properties with anyOf
as you could be multiple of the referenced types at once, where we intend it to be oneOf
the referenced types)
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.
@Yannick-Malins The change I'm after is on line 36 above. Otherwise, this is good to merge, apart from a Changelog entry and the $id
I flagged above.
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.
LGTM - although I think Message's entities element needs to be defined with oneOf rather than anyOf. Could you change that, add a Changelog entry and then I'll approve and merge the change (new governance restrictions will invalidate any reviews that happen before a later change)
@@ -0,0 +1,53 @@ | |||
{ | |||
"$schema": "http://json-schema.org/draft-07/schema#", | |||
"$id": "https://fdc3.finos.org/schemas/next/context/attachment.schema.json", |
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.
the filename and this $id
need to match, please change to: https://fdc3.finos.org/schemas/next/context/fileAttachment.schema.json
@Yannick-Malins lemme know if you can make these two quick changes (I didn't add suggestions or I won't be able to review the change after you merge them) - alternatively let me know to re-raise this PR and have you review instead. |
#1255