casey / intermodal

A command-line utility for BitTorrent torrent file creation, verification, and more
https://imdl.io
Creative Commons Zero v1.0 Universal
487 stars 25 forks source link

Add FromStr for MagnetLink #482

Closed atomgardner closed 4 years ago

atomgardner commented 4 years ago

Another step toward #255.

casey commented 4 years ago

I think that FromStr is too general for MagnetLink, perhaps it should be TryFrom<URL>? I would also make a separate error type, since it seems there are a lot of ways that magnet link parsing can fail, perhaps MagnetLinkParseError?

atomgardner commented 4 years ago

I think FromStr was suggested so the subcommand would be nice and clean:

#[derive(StructOpt)]
#[structopt(...)]
pub(crate) struct FromLink {
  link : MagnetLink
}

Is StructOpt smart enough to walk the &str -> URL -> MagnetLink parser lattice without any guidance?

casey commented 4 years ago

Ahh, you're right. In that case, I think FromStr is fine.

casey commented 4 years ago

Also, just thought of this, I think it's allowed for a magnet link to have multiple exact topics. There are different xt types for different networks, and you're allowed to have multiple xts:

http://wiki.depthstrike.com/wiki/P2P:Protocol:Specifications:Magnet#xt_-_eXact_Topic

I think the right behavior is to just grab the first xt key where the value starts with urn:btih:.

atomgardner commented 4 years ago

have sent a band-aid patch (#485) for the linting failures

casey commented 4 years ago

I just merged #485, although with some changes. Let me know when this is ready to review!

casey commented 4 years ago

Looks good! Can you flip the switch that lets me push to this branch? I need to push a version with my PGP signature attached in order to merge it.

casey commented 4 years ago

I just realized a couple things that could be improved:

  1. There's an assert_matches macro that can be used to clean up the tests. You can do assert_matches!(VALUE, PATTERN if CONDITION).

  2. I think all the logic inside the fromstr implementation could be moved into MagnetLink::parse(text: &str) -> Result<MagnetLink, MagnetLinkParseError> and then from_str could just be Self::parse(text).context(error::MagnetLinkParse{text}), to avoid a bunch of .context calls.

Feel free to do these, or I can do them before I merge.

atomgardner commented 4 years ago

...to avoid a bunch of context calls.

excellent! :100: :100:

Also, you now have permission to clobber this branch.

casey commented 4 years ago

Sweet, merged! Thank you!