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

Issue deprecation warning on Code if identity feature is enabled #260

Closed

Conversation

thomaseizinger
Copy link
Contributor

This is a PoC for the discussion in #201 (comment).

@Stebalien
Copy link
Member

Nice!

Will this get triggered in a dependency? I.e.:

  • I start with A -> B -> multihash.
  • Then I add A -> C -> multihash ("identity").

@thomaseizinger
Copy link
Contributor Author

Nice!

Will this get triggered in a dependency? I.e.:

* I start with `A -> B -> multihash`.

* Then I add `A -> C -> multihash ("identity")`.

As soon as the feature is enabled in the dependency graph, your usage of Code will be flagged as deprecated. Starting from there, your code (pun intended) can trigger the foot-gun. What might be annoying is that users cannot turn off the deprecation warning if a newly added dependency turns in on unconditionally. For those cases, they can say #[allow(deprecated] if they are sure that they never pass 0 into the try_from function.

@Stebalien
Copy link
Member

I see. That definitely helps, although I'm still concerned about the case where the user doesn't actually use Code. They just happen to import two crates that do, and one of these crates enables this feature (breaking the other crate).

@thomaseizinger
Copy link
Contributor Author

thomaseizinger commented Dec 21, 2022

I see. That definitely helps, although I'm still concerned about the case where the user doesn't actually use Code. They just happen to import two crates that do, and one of these crates enables this feature (breaking the other crate).

Warnings that entirely appear within dependencies are silenced by cargo as far as I know. If you compile a project with max verbosity, I think you can still see them.

There is obviously still the problem where the foot-gun sits within a dependency and is triggered there.

One could argue that shipping a backwards-compatible deprecation warning first will make it easier for users to upgrade and thus have them see it earlier.

@Stebalien
Copy link
Member

Yeah, I agree. Let's do both.

@thomaseizinger
Copy link
Contributor Author

Before we ship this, let me remove our dependency on it in rust-libp2p and cut a patch release, otherwise we might trigger exactly what we just said: A deprecation warning that users can't fix themselves.

@thomaseizinger
Copy link
Contributor Author

Closed in favor of #289.

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.

2 participants