Skip to content
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

Add missing argument in bridging message for findIntentByContext #1212

Merged
merged 1 commit into from
May 28, 2024

Conversation

kriswest
Copy link
Contributor

resolves #1211

@kriswest kriswest requested review from robmoffat and a team May 14, 2024 14:22
Copy link

netlify bot commented May 14, 2024

Deploy Preview for fdc3 ready!

Name Link
🔨 Latest commit badf69b
🔍 Latest deploy log https://app.netlify.com/sites/fdc3/deploys/664373aeaeeea80008be3758
😎 Deploy Preview https://deploy-preview-1212--fdc3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@kriswest kriswest requested a review from hughtroeger May 20, 2024 09:51
@@ -5280,6 +5282,7 @@ const typeMap: any = {
], false),
"FindIntentsByContextAgentRequestPayload": o([
{ json: "context", js: "context", typ: r("ContextElement") },
{ json: "resultType", js: "resultType", typ: u(undefined, "") },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for my own understanding, I was wondering if typ: u(undefined, "") violates the 'no unions of primitive types' guidelines from this PR #1200 , but I assume this is how optional parameters are expressed here, and this is fine because it's really just an optional string.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know its fine in this case as we already have optional parameters elsewhere and those can generally be implemented via alternative function signatures in other languages. I'd probably also exclude null from the set of primitive types when considering this as most languages have someway of supporting that (e.g. Nullable<int> i = null; in C#, or using the Integer class in Java instead of int.

@kriswest kriswest merged commit e931c96 into main May 28, 2024
10 checks passed
@kriswest kriswest deleted the fix-bridging-findIntentsByContext branch May 28, 2024 18:10
@kriswest
Copy link
Contributor Author

This PR when in without a changelog entry, which we will need to add before the next version of FDC3 is adopted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FindIntentsByContextAgentRequest doesn't allow for supplying resultType
3 participants