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

Move FilesystemExporter from trait object to generic #6025

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

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
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