Skip to content

FIXME: Make TProtocol Optional[TProtocol] to keep types consistent #770

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Jineshbansal
Copy link
Contributor

What was wrong?

This PR aims to make TProtocol as Optional[TProtocol] to keep types consistent in py-libp2p/libp2p/protocol_muxer/multiselect.py

@Jineshbansal
Copy link
Contributor Author

Jineshbansal commented Jul 15, 2025

Please take a look at this @seetadev , If you tell me some pointers on how to check its reliability, that would be really helpful. Currently I just change the TProtocol as Optional[TProtocol] and totally rely on tests ("tests/core/protocol_muxer/test_protocol_muxer.py", pre-commit tests) to make further changes.

@seetadev
Copy link
Contributor

@Jineshbansal : Great, thank you so much for sharing the PR. Appreciate it.

Definitely, I'll review it.

CCing @sumanjeet0012 and @acul71 for their feedback and review too.

@acul71
Copy link
Contributor

acul71 commented Jul 17, 2025

I've seen that windows test failing randomly in other PRs
FAILED tests/core/pubsub/test_gossipsub_px_and_backoff.py::test_peer_exchange

@Jineshbansal
Copy link
Contributor Author

@acul71 thanks for pointing it out, I will definitely look into this

@seetadev
Copy link
Contributor

@acul71 , @Jineshbansal : Not related, we can ignore this test case. CCing @mystical-prog, who could help fix it via a PR.

@Jineshbansal : Please add newsfragment and more test cases.

@acul71
Copy link
Contributor

acul71 commented Aug 12, 2025

@Jineshbansal @seetadev
What's the current status of this ? @Jineshbansal are you still woking on this?

@Jineshbansal
Copy link
Contributor Author

@acul71 , sorry I couldn't get a time for this, I will finalize my changes in this week

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.

3 participants