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: derive Clone for mdns::Event #3606

Merged
merged 7 commits into from
Mar 13, 2023
Merged

feat: derive Clone for mdns::Event #3606

merged 7 commits into from
Mar 13, 2023

Conversation

drHuangMHT
Copy link
Contributor

@drHuangMHT drHuangMHT commented Mar 13, 2023

Description

Derive trait Clone for mdns::Event. This makes cloning its contents without destroying type information possible.

Related #3593.

Notes & open questions

CHANGELOG to be filled. Should we open a new minor version for the change?

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

@drHuangMHT
Copy link
Contributor Author

Hmm why the check failed? Is it a timeout?

@thomaseizinger
Copy link
Contributor

Hmm why the check failed? Is it a timeout?

The WebRTC tests are flaky unfortunately. It is an upstream bug that is fixed in the latest version but we can't upgrade at the moment. See webrtc-rs/webrtc#413.

I restarted the job, it will likely pass now.

@thomaseizinger thomaseizinger changed the title feat: make mdns::Event clone-able feat: derive Clone for mdns::Event Mar 13, 2023
@thomaseizinger
Copy link
Contributor

CHANGELOG to be filled. Should we open a new minor version for the change?

Patch version bump is good enough as this isn't breaking.

Unreleased patch version 0.43.1 bumped. Added entry for PR 3606: Deriving 'Clone' for 'mdns::Event'
thomaseizinger
thomaseizinger previously approved these changes Mar 13, 2023
@thomaseizinger thomaseizinger marked this pull request as ready for review March 13, 2023 16:17
@mergify mergify bot dismissed thomaseizinger’s stale review March 13, 2023 16:18

Approvals have been dismissed because the PR was updated after the send-it label was applied.

@mergify mergify bot merged commit 9d05c61 into libp2p:master Mar 13, 2023
@@ -1,3 +1,8 @@
# 0.43.1 [unreleased]
Copy link
Member

Choose a reason for hiding this comment

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

This missed a version bump in protocols/mdns/Cargo.toml. I will make the bump with the upcoming release pull request.

Copy link
Member

Choose a reason for hiding this comment

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

Bump happening through #3694.

mxinden added a commit to mxinden/rust-libp2p that referenced this pull request Mar 29, 2023
mergify bot pushed a commit that referenced this pull request May 2, 2023
In previous PR #3606 we've made `mdns::Event` `Clone`, but cloning single-use iterators doesn't sound right. Also you have to create an iterator from the actual data returned before putting it into events. So in this PR the iterators are replaced by `Vec`, as it's the type the data originally come from.



Related #3612.

Pull-Request: #3621.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants