search

multiformats / rust-multiaddr

multiaddr implementation in rust
https://crates.io/crates/multiaddr
Other
86 stars 45 forks source link

Implement missing protocols #110

Open Zollerboy1 opened 3 months ago

Zollerboy1 commented 3 months ago

This PR implements all protocols listed in https://github.com/multiformats/multiaddr/blob/master/protocols.csv.

Presumably, this PR closes #97.

umgefahren commented 3 months ago

I would like to see that merged especially since we need ip6zone

umgefahren commented 2 months ago

What's the status on this? @jxs

jxs commented 1 month ago

Hi @umgefahren sorry I missed this notification, and I don't seem to have permissions for this repo, I can ask to, meanwhile @Zollerboy1 sorry for the delay, but can we separate this into two PR's one implementing the missing protocols and the other re-ordering them? As it is it's very hard to review. Thanks!

Zollerboy1 commented 1 month ago

Hey @jxs, of course, I can do that. I'm on vacation right now, so I probably cannot work on this until next week though.

Zollerboy1 commented 1 month ago

Hey @jxs, I finally came around to do this, it should be much easier to review now.

I have the code with the reordered protocols in another branch and would create a new PR when (or if) this one gets merged.

Currently, the CI is failing because the new rust-check-cfg feature in Rust 1.80 complains about the use of #[cfg(nightly)] in the test file. I wasn't sure how exactly you'd want to solve this, but this should probably be changed to #[cfg(feature = "nightly")] or similar.

umgefahren commented 3 weeks ago

Currently, the CI is failing because the new rust-check-cfg feature in Rust 1.80 complains about the use of #[cfg(nightly)] in the test file. I wasn't sure how exactly you'd want to solve this, but this should probably be changed to #[cfg(feature = "nightly")] or similar.

I don't think adding an extra feature that only pertains to a test is a good idea. Using this cfg is actually not that uncommon. I would suggest you add the lint exception:

[lints.rust]
unexpected_cfgs = { level = "warn", check-cfg = ['cfg(nightly)'] }

It might seem like a dirty fix, but it's a widely used workaround:

It's also one of the suggested solutions by rust itself (and it's more efficient then using a build.rs file).

umgefahren commented 3 weeks ago

I made a quick PR: #112

jxs commented 2 weeks ago

Hi @Zollerboy1 can you pull the latest changes so we can make CI pass? Thanks!

umgefahren commented 2 weeks ago

Hi @Zollerboy1 can you pull the latest changes so we can make CI pass? Thanks!

It passes now.