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

feat: Re-export x25519_dalek #312

Merged
merged 1 commit into from
Dec 16, 2022

Conversation

lu-zero
Copy link
Contributor

@lu-zero lu-zero commented Aug 29, 2022

This way downstream crates not have to explicitly import the
x25519_dalek crate and sync it up when needed.

@Noah-Kennedy
Copy link
Collaborator

I'm not really a fan of the whole pattern of re-exporting stuff from other crates unless there is a good reason to do so, like preventing naming collisions.

@lu-zero
Copy link
Contributor Author

lu-zero commented Aug 30, 2022

You are using those types as key part of your API, users cannot opt out and they have to ensure that they keep the two crates in sync in their Cargo.toml. It is far from pleasant over time.

@nappa85
Copy link

nappa85 commented Aug 30, 2022

I've the same problem with other crates, first of all opentelemetry-otlp using an old tonic version and not re-exporting it.
If you're exposing another crate types, it's a best practice to re-export the crate to avoid people to chase the version you're using, for you it's a simple pub use, for us it's a lot less stress

@lu-zero
Copy link
Contributor Author

lu-zero commented Dec 13, 2022

Are there any chances to consider this patch? I can rebase it if that's the case.

@Noah-Kennedy
Copy link
Collaborator

I think I'm fine with this. If you resolve the conflicts, I'll sign off.

This way downstream crates not have to explicitly import the
x25519_dalek crate and sync it up when needed.
@Noah-Kennedy Noah-Kennedy merged commit 5d4dac2 into cloudflare:master Dec 16, 2022
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.

None yet

3 participants