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

[c#] support for system.collections.immutable collections #1161

Merged

Conversation

enioluwas
Copy link
Contributor

@enioluwas enioluwas commented Aug 12, 2022

Closes #1159

Adds proper support for immutable collections as laid out in the issue description. Implemented this in a way such that the Bond C# package doesn't actually depend on System.Collections.Immutable, roughly following how it was done in Newtonsoft.Json.

The collections are constructed using the builders, which performs a lot better doing bulk operations.

The change to the codegen in Haskell is small, but since I don't have much experience, any suggestions to make it more idiomatic are welcome.

Added unit tests both for the compiler and for [de]serialization in C#.

@enioluwas enioluwas force-pushed the dev/esegun/immutable-collections-support branch 3 times, most recently from 6e27051 to bda50dd Compare August 15, 2022 21:43
@chwarr
Copy link
Member

chwarr commented Aug 16, 2022

Thanks for the PR!

I have other commitments that I need to attend to at the moment, so I won't be able to review this PR for a while. It's on my to do list, however. I cannot give an estimate on when I will be able to review this, unfortunately.

@enioluwas
Copy link
Contributor Author

Hey @chwarr any chance you could prioritize reviewing this? My team already depends on Bond for schema definitions, and partially for serialization. This feature is the last blocker in moving all our serialization to Bond, which would be a performance boost for us over JSON. Thanks!

Copy link
Member

@chwarr chwarr left a comment

Choose a reason for hiding this comment

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

Overall, LGTM. Thank you for the PR and for your patience.

Can you add:

  • an example project,
  • a note in the documentation (under the Custom Containers section?) about the special handing for the immutable types, and
  • an entry in the changelog under Unreleased.

Thanks.

@enioluwas enioluwas force-pushed the dev/esegun/immutable-collections-support branch from bda50dd to c8c2a7d Compare November 23, 2022 21:36
@enioluwas
Copy link
Contributor Author

Overall, LGTM. Thank you for the PR and for your patience.

Can you add:

  • an example project,
  • a note in the documentation (under the Custom Containers section?) about the special handing for the immutable types, and
  • an entry in the changelog under Unreleased.

Thanks.

Done! Added the immutable_collections example project. Let me know if the wording on any of the doc changes needs to be modified

@enioluwas enioluwas requested a review from chwarr November 28, 2022 15:09
Copy link
Member

@chwarr chwarr left a comment

Choose a reason for hiding this comment

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

I'll merge when the CI build for my minor changes passes.

@chwarr chwarr merged commit b13fee1 into microsoft:master Nov 30, 2022
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.

[C#] Support System.Collections.Immutable collections as custom containers
2 participants