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

protocols/ping: Properly deprecate types with Ping prefix #2937

Merged
merged 6 commits into from
Sep 30, 2022

Conversation

thomaseizinger
Copy link
Contributor

Description

The deprecation we tried to do in #2215 doesn't work but defining type aliases works :)

Links to any relevant issues

Open Questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

Hi Thomas, when doing #2927 noticed there are still some references to the deprecated types in the docs, and some types with Ping prefix, should they be addressed?

(sorry for doing it like this, github still doesn't support reviewing on unchanged files nor unchanged code)

docs:

//! The [`Ping`] struct implements the [`NetworkBehaviour`] trait. When used with a [`Swarm`],

//! The `Ping` network behaviour produces [`PingEvent`]s, which may be consumed from the `Swarm`

//! > by default, see [`PingConfig::with_keep_alive`] for changing this behaviour.

/// Creates a new `PingConfig` with the following default settings:

/// Builds a new `PingHandler` with the given configuration.

structs:

enum PingState {

type PingFuture = BoxFuture<'static, Result<(NegotiatedSubstream, Duration), io::Error>>;

@elenaf9
Copy link
Contributor

elenaf9 commented Sep 27, 2022

What do you think about just removing the deprecated types? They have been marked as deprecated for quite some time now, and upgrading to the new names should be easy.

@thomaseizinger
Copy link
Contributor Author

What do you think about just removing the deprecated types? They have been marked as deprecated for quite some time now, and upgrading to the new names should be easy.

They have been incorrectly marked as deprecated :)

@elenaf9
Copy link
Contributor

elenaf9 commented Sep 27, 2022

What do you think about just removing the deprecated types? They have been marked as deprecated for quite some time now, and upgrading to the new names should be easy.

They have been incorrectly marked as deprecated :)

Incorrectly in the sense that users that were still using them did not get a warning? Does the deprecated macro not work on re-exports?

@jxs
Copy link
Member

jxs commented Sep 27, 2022

What do you think about just removing the deprecated types? They have been marked as deprecated for quite some time now, and upgrading to the new names should be easy.

They have been incorrectly marked as deprecated :)

Incorrectly in the sense that users that were still using them did not get a warning? Does the deprecated macro not work on re-exports?

Hi Elena, yeah exactly. See #2927 (comment)

Co-authored-by: Elena Frank <elena.frank@protonmail.com>
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for making it easier for users to upgrade.

If I am not mistaken, this is missing the suggestions by @jxs. Otherwise looks good to me.

@thomaseizinger
Copy link
Contributor Author

If I am not mistaken, this is missing the suggestions by @jxs. Otherwise looks good to me.

Yep! Unfortunately, rustdoc doesn't flag links to deprecated items as deprecated. Grml.

@thomaseizinger
Copy link
Contributor Author

Okay, I went across the repo again with fulltext search and found quite a few more usages.

I also realised that our CI doesn't check for warnings in examples! I'll see to fix that too.

@thomaseizinger
Copy link
Contributor Author

I also realised that our CI doesn't check for warnings in examples! I'll see to fix that too.

Done: #2949

Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

Okay, I went across the repo again with fulltext search and found quite a few more usages.

I also realised that our CI doesn't check for warnings in examples! I'll see to fix that too.

awesome, LGTM!
Did you see the references to PingState and PingFuture ? PingState can be renamed to just State as it isn't shared outside of the module. though PingFuture and PongFuture probably there isn't a better wording for them.

@thomaseizinger
Copy link
Contributor Author

Okay, I went across the repo again with fulltext search and found quite a few more usages.
I also realised that our CI doesn't check for warnings in examples! I'll see to fix that too.

awesome, LGTM! Did you see the references to PingState and PingFuture ? PingState can be renamed to just State as it isn't shared outside of the module. though PingFuture and PongFuture probably there isn't a better wording for them.

I guess PingState could be renamed yeah. I think public API types are the most important. Everything else we can any time :)

@thomaseizinger thomaseizinger merged commit 1da75b2 into master Sep 30, 2022
@thomaseizinger thomaseizinger deleted the deprecate-ping branch November 17, 2022 01:18
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.

4 participants