Skip to content
This repository has been archived by the owner on Jan 25, 2024. It is now read-only.

feat: add quic/tcp features and set quic as default #22

Merged
merged 1 commit into from
Jan 18, 2024

Conversation

joshuef
Copy link
Contributor

@joshuef joshuef commented Jan 17, 2024

No description provided.

src/local.rs Outdated
"/ip4/127.0.0.1/tcp/{port}/p2p/{peer_id}"
))?)
} else
// efault to quic
Copy link

Choose a reason for hiding this comment

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

Typo. Maybe also use #[cfg(feature = "quic")] like below to keep it consistent.

Copy link

Choose a reason for hiding this comment

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

That should also force this library to be compiled with either quic or tcp, because it's not possible to compile without either feature.

src/service.rs Outdated
} else if cfg!(feature = "quic") {
UdpSocket::bind(("127.0.0.1", port)).is_ok()
} else {
false
Copy link

Choose a reason for hiding this comment

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

Compiling this crate without either tcp and quic feature doesn't really make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

aye, the compiler complained that it could still. I'm not sure of the best default strat here 🤔

src/local.rs Outdated
"/ip4/127.0.0.1/tcp/{port}/p2p/{peer_id}"
))?)
if cfg!(feature = "tcp") {
#[cfg(feature = "tcp")]
Copy link

Choose a reason for hiding this comment

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

Double feature gate here?

@joshuef joshuef merged commit d644c5f into maidsafe:main Jan 18, 2024
13 checks passed
@joshuef joshuef deleted the QuicFeats branch January 18, 2024 08:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants