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 list element #233

Merged
merged 3 commits into from
Feb 29, 2024
Merged

Add list element #233

merged 3 commits into from
Feb 29, 2024

Conversation

simonbyford
Copy link
Contributor

@simonbyford simonbyford commented Jan 31, 2024

What does this change?

Adds a new element type List which will be used to provide more structure around existing formats such as Key Takeaways.

This block element is novel in that it also contains block element, creating a recursive data structure. It is not currently anticipated that a List element will contain List elements, although there is no easy way to prevent this in the thrift definition.

The recursion did cause problems with Fezziwig's auto serialisation, which we have worked around by explicitly caching the serialisers for BlockElement. It is possible that we will change our approach here following work on Fezziwig itself by Emily Bourke.

How to test

Run tests, they should pass.

publishLocal and try building/testing content-api-scala-client and content-api with this library.

  • concierge
  • content-api-scala-client

@davidfurey davidfurey marked this pull request as ready for review February 21, 2024 10:01
@davidfurey davidfurey requested a review from a team as a code owner February 21, 2024 10:01
@davidfurey davidfurey requested review from a team and removed request for a team February 21, 2024 10:01
Copy link
Contributor Author

@simonbyford simonbyford left a comment

Choose a reason for hiding this comment

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

Looks good to me. Will need approval from someone else in the team!

@davidfurey
Copy link
Member

This change has been tested locally with data flowing from composer through to concierge

@emdash-ie
Copy link
Contributor

Can you make a beta release without merging and test with that, as Justin suggests in chat?

Copy link

@davidfurey has published a preview version of this PR with release workflow run #49, based on commit 7c9ce77:

19.0.0-PREVIEW.add-list-element.2024-02-27T1153.7c9ce77e

Want to make another preview release?

Click 'Run workflow' in the GitHub UI, specifying the add-list-element branch, or use the GitHub CLI command:

gh workflow run release.yml --ref add-list-element

Want to make a full release after this PR is merged?

Click 'Run workflow' in the GitHub UI, leaving the branch as the default, or use the GitHub CLI command:

gh workflow run release.yml

@davidfurey
Copy link
Member

This has been tested locally and now in CODE with the preview release

@davidfurey davidfurey merged commit 46bb419 into main Feb 29, 2024
9 checks passed
@davidfurey
Copy link
Member

davidfurey commented Apr 24, 2024

See also https://stackoverflow.com/questions/34399288/caching-implicit-resolution

(although implicit shadowing isn't a thing in Scala 3 - scala/scala3#5887)

@mxdvl mxdvl deleted the add-list-element branch April 24, 2024 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants