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

chore(perf): remove unused module #4655

Merged
merged 4 commits into from
Oct 16, 2023

Conversation

leonzchang
Copy link
Contributor

Description

Related: #3844.

Notes & 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

@leonzchang
Copy link
Contributor Author

Since this PR doesn't change any public API, I think bump the patch version will be ok?

@thomaseizinger
Copy link
Contributor

Thanks for sending this!

I just had a look at the code and we seem to have a duplicate implementation here? Everything in client/ and server/ is actually unused. My IDE flags the modules as not included in the module tree.

Thus I think what you can do is simply delete these directories :)
Because the code is not included, we don't even need to bump version here.

@leonzchang
Copy link
Contributor Author

Thanks for sending this!

I just had a look at the code and we seem to have a duplicate implementation here? Everything in client/ and server/ is actually unused. My IDE flags the modules as not included in the module tree.

Thus I think what you can do is simply delete these directories :) Because the code is not included, we don't even need to bump version here.

Ok, fixed b77234f, will move on to the rest protocols

@leonzchang leonzchang changed the title feat(perf): remove KeepAlive::Until feat(perf): remove unused module Oct 16, 2023
@thomaseizinger thomaseizinger changed the title feat(perf): remove unused module chore(perf): remove unused module Oct 16, 2023
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Always great to delete code! :)

Thanks!

@thomaseizinger
Copy link
Contributor

cc @mxinden This seems to have been an oversight when you refactored it to use libp2p-request-response! I am not sure how close #4382 is to be mergable but if that needs any of this code again, it can always just re-introduce it.

For now, deleting this allows us to make progress on #3844.

@thomaseizinger
Copy link
Contributor

Hmm, this new changelog lint that I introduced does have some funny side-effects in regards to non-changelog-worthy changes like this one.

@thomaseizinger
Copy link
Contributor

Hmm, this new changelog lint that I introduced does have some funny side-effects in regards to non-changelog-worthy changes like this one.

Okay, I have a temporary fix in #4658.

@mergify mergify bot merged commit e1c1f03 into libp2p:master Oct 16, 2023
73 checks passed
@mxinden
Copy link
Member

mxinden commented Oct 16, 2023

Thank you for the clean-up @leonzchang!

@thomaseizinger agreed, don't need to wait for #4382.

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