Skip to content

Move FilesystemExporter from trait object to generic #6025

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

Closed

Conversation

Manishearth
Copy link
Member

I don't know why we wanted to do this, but it's listed in #2856 so I did it. Feel free to close if we don't want to do it anymore.

@Manishearth Manishearth requested review from sffc and a team as code owners January 22, 2025 08:14
@Manishearth Manishearth mentioned this pull request Jan 22, 2025
37 tasks
@sffc
Copy link
Member

sffc commented Jan 22, 2025

Did some archaeology and it seems I added this to the checklist on September 12, 2023. I don't see any PRs that landed within a week of that date that would have caused me to add this.

I think what I was probably thinking was:

  1. The serializers module has always been a bit clunky and it would be nice if AbstractSerializer weren't public
  2. We only really support a small number of formats: JSON, Postcard v1, and Bincode v1. We haven't tested it with other formats, and Serde doesn't guarantee that it will work with other formats.
  3. We don't like trait objects, so this is clearly an opportunity to move away from it
  4. So, maybe we should move away from the trait and instead just have these be options. In other words, change this from trait object to enum, not a generic.

I don't think I would have considered a generic to be a better situation than the trait object, because the generic probably increases code size in this situation, since the generic function is deep, not shallow.

@sffc
Copy link
Member

sffc commented Jan 22, 2025

I suggest we make this a 2.0-stretch issue and discuss it there.

@Manishearth Manishearth marked this pull request as draft January 23, 2025 01:05
@sffc sffc added the discuss Discuss at a future ICU4X-SC meeting label Jan 23, 2025
@sffc
Copy link
Member

sffc commented Mar 10, 2025

The trait AbstractSerializer returns a BufferFormat, which is an enum we own, so it isn't really like someone could come along and add their own serde backend, and if they wanted one, we should probably test it, first.

So I don't think we should land this PR, but we might want to land one that makes AbstractSerializer private.

Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

^

Manishearth added a commit that referenced this pull request Mar 11, 2025
Supersedes #6025

Can't be private, it's used in FilesystemExporter::try_new

<!--
Thank you for your pull request to ICU4X!

Reminder: try to use [Conventional
Comments](https://conventionalcomments.org/) to make comments clearer.

Please see
https://github.com/unicode-org/icu4x/blob/main/CONTRIBUTING.md for
general
information on contributing to ICU4X.
-->

---------

Co-authored-by: Shane F. Carr <[email protected]>
@Manishearth Manishearth deleted the serializer-trait-object branch April 2, 2025 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Discuss at a future ICU4X-SC meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants