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

remove union type for chat message #1102

Merged
merged 7 commits into from
Dec 13, 2023

Conversation

Yannick-Malins
Copy link
Contributor

force the message part of a chat message to be a types structure, remove the union type that was kept for initial compatibility

update documentation

Copy link

linux-foundation-easycla bot commented Nov 10, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

netlify bot commented Nov 10, 2023

Deploy Preview for fdc3 canceled.

Name Link
🔨 Latest commit 0505a4d
🔍 Latest deploy log https://app.netlify.com/sites/fdc3/deploys/6579b227d8d44e0008d7a657

@kriswest kriswest self-requested a review November 10, 2023 14:58
@kriswest kriswest added context-data FDC3 Context Data Working Group Context Data & Intents Contexts & Intents Discussion Group bug Something isn't working labels Nov 10, 2023
@kriswest
Copy link
Contributor

@bingenito Heres a PR from @Yannick-Malins that will remove that union type, which was left in for backwards compatibility, but is likely not needed as this type was added in 2.1. If this looks good to you we'll confirm consent for it at the next SWG meeting and then merge it (and regenerate the typescript context types in the NPM module - currently a manual process due to a quicktype issue).

I'll also see what we can do about adding a recommendation to not use union types in future.

@bingenito
Copy link
Member

(and regenerate the typescript context types in the NPM module - currently a manual process due to a quicktype issue).

Is it possible to see what the type will look like as part of the PR?

@kriswest
Copy link
Contributor

(and regenerate the typescript context types in the NPM module - currently a manual process due to a quicktype issue).

Is it possible to see what the type will look like as part of the PR?

Not until I can migrate it to FINOS repo branch and run the generation (would be automated, but for the aforementioned quicktype issue). I can do that migration after @Yannick-Malins is authorized by easyCLA (which is hopefully just a case of clicking on the link in the above message from the easyCLA bot #1102 (comment))

@kriswest
Copy link
Contributor

This is the quicktype PR we're waiting on seeing merged so we can go back to automated generation: glideapps/quicktype#2426
I've given them a nudge on it

@kriswest
Copy link
Contributor

@bingenito I got access to the fork from @Yannick-Malins to regenerate the types, you can now see the change to ContextTypes.ts in the diff

@bingenito
Copy link
Member

Thanks @kriswest and @Yannick-Malins, looks good. I could have inferred that is the result but nice to see what gets generated up front.

@kriswest
Copy link
Contributor

Thanks @kriswest and @Yannick-Malins, looks good. I could have inferred that is the result but nice to see what gets generated up front.

Very true - feel free to bug the quicktype maintainers for me :-p

@kriswest kriswest added the needs-vote Please vote on this issue label Nov 13, 2023
@kriswest
Copy link
Contributor

kriswest commented Dec 4, 2023

Generation of types is back to automated on commit, although it doesn't make it into the PR unless you run it yourself first, which is now also possible.

@Yannick-Malins the SWG meeting approved doing this last week, so now we just need to:

@kriswest
Copy link
Contributor

/easycla

@kriswest
Copy link
Contributor

@Yannick-Malins @mistryvinay can one of you hit that authorize link in the EasyCLA comment and check if you've got one of the new CLAs in place?

Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

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

Needs a changelog entry

@kriswest kriswest self-requested a review December 13, 2023 13:29
Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

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

LGTM

@kriswest kriswest merged commit 4c231dd into finos:master Dec 13, 2023
10 checks passed
@kriswest kriswest deleted the remove_chat_message_string branch December 13, 2023 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Context Data & Intents Contexts & Intents Discussion Group context-data FDC3 Context Data Working Group needs-vote Please vote on this issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants