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

Exposing diplomat-tool through a library #348

Merged
merged 2 commits into from
Oct 21, 2023
Merged

Exposing diplomat-tool through a library #348

merged 2 commits into from
Oct 21, 2023

Conversation

robertbastian
Copy link
Collaborator

@robertbastian robertbastian commented Oct 20, 2023

Clients like ICU4X need to version diplomat-tool along with the other diplomat crates. Cargo cannot at the moment version binaries, exposing a library is the recommended workaround (rust-lang/cargo#2267).

@Manishearth
Copy link
Contributor

I'd rather not commit to an API here just yet; especially since a thing I definitely want to do is give backends the ability to accept parameters.

@sffc
Copy link
Contributor

sffc commented Oct 20, 2023

It's pre-1.0 so personally I don't have a huge concern with making a starter API


pub fn gen(
entry: &Path,
target_language: &str,
Copy link
Contributor

Choose a reason for hiding this comment

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

at the very least I think we should have a config struct or something that encapsulates all this

Copy link
Contributor

Choose a reason for hiding this comment

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

we should also perhaps have a CLI feature

@Manishearth
Copy link
Contributor

Hm, fair. I do think we should spend some effort cleaning up the API though

@robertbastian
Copy link
Collaborator Author

There's a lot to clean up in Diplomat. One thing I noticed is that the language gets passed around everywhere as a &str instead of an enum, so it's hard to give this a nicer API without bigger changes.

@robertbastian robertbastian merged commit 354307d into main Oct 21, 2023
5 checks passed
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