rust-multiaddr icon indicating copy to clipboard operation
rust-multiaddr copied to clipboard

Implement missing protocols

Open Zollerboy1 opened this issue 1 year ago • 7 comments

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

Presumably, this PR closes #97.

Zollerboy1 avatar May 21 '24 01:05 Zollerboy1

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

umgefahren avatar May 24 '24 10:05 umgefahren

What's the status on this? @jxs

umgefahren avatar Jun 09 '24 12:06 umgefahren

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!

jxs avatar Jul 09 '24 18:07 jxs

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 avatar Jul 10 '24 10:07 Zollerboy1

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.

Zollerboy1 avatar Jul 27 '24 21:07 Zollerboy1

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 avatar Aug 02 '24 14:08 umgefahren

I made a quick PR: #112

umgefahren avatar Aug 02 '24 14:08 umgefahren

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

jxs avatar Aug 14 '24 16:08 jxs

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

It passes now.

umgefahren avatar Aug 14 '24 21:08 umgefahren