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

Set up basic diplomat workflow #163

Merged
merged 7 commits into from
Jan 21, 2025
Merged

Set up basic diplomat workflow #163

merged 7 commits into from
Jan 21, 2025

Conversation

Manishearth
Copy link
Contributor

Progress on #104

This adds a simple diplomat integration, but does not use it yet. When we start using it we should add CI that runs cargo run -p diplomat-gen and ensures there is no diff.

@Manishearth
Copy link
Contributor Author

I added a bunch of bindings! No tests yet, and I don't yet check in any bindings.

Copy link
Member

@jasonwilliams jasonwilliams left a comment

Choose a reason for hiding this comment

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

LGTM, I've ran cargo run -p diplomat-gen and seen the header files it generates. Looks like a good start. Ideally we may want a contributing guide to make sure the wrapper lib is updated alongside the underlying temporal_rs code but it sounds like some CI can be added for that. I also don't think that's a blocker for this.

temporal_capi/src/calendar.rs Show resolved Hide resolved
@Manishearth
Copy link
Contributor Author

@jasonwilliams yep, as I mention in the issue, my plan is to do that in future PRs. My goal is to get this in a place where collaboration can happen, we can add CI and proper tests later.

@nekevss
Copy link
Member

nekevss commented Jan 20, 2025

This is looking great! Thanks for looking into this and helping out! Out of curiosity, have you been able to take a look at #169? If I'm understanding diplomat correctly, I don't think that it will affect any subsequent work related to diplomat, but I wanted to confirm first.

@Manishearth
Copy link
Contributor Author

@nekevss it shouldn't matter, exposing it over FFI will be additional work but that's about it.

One thing that could be tricky is if you ever decided to expose data sources for calendars: currently we're able to cheaply clone Calendar everywhere (both temporal_rs and temporal_capi do this) because we know they're compiled-data backed, changing this might require wrapping them in an Arc or something.

@jedel1043
Copy link
Member

jedel1043 commented Jan 20, 2025

@Manishearth How does ICU4X expose provider APIs? Because I was thinking we could change TimeZoneProvider to be a singleton DataProvider that returns some kind of tzif-like struct, which could make it much easier to expose through FFI if ICU4X already has a way to do that. And if we can do that, we can adapt that solution to also work with calendars.

@Manishearth
Copy link
Contributor Author

We have _with_provider APIs over FFI that take a &DataProvider, which is not persistently borrowed: it returns payloads that contain Arc/Rc.

@Manishearth
Copy link
Contributor Author

FWIW my preference is for this to be merged, I could push more APIs, and might do that on my flight tomorrow, but it will be nice to have something in.

@nekevss
Copy link
Member

nekevss commented Jan 21, 2025

This looks good to me! 😄 Just needs a rebase / merge with main

@Manishearth
Copy link
Contributor Author

Oh, I didn't realize those were mandatory. Done!

@nekevss nekevss merged commit 96e16b1 into boa-dev:main Jan 21, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants