Add FromStr for MagnetLink #482

atomgardner · 2020-10-03T03:29:22Z

Another step toward #255.

casey · 2020-10-03T23:09:13Z

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?

casey

Also, it would be good to add tests that check that the expected errors are returned when malformed magnet links are parsed.

src/error.rs

casey · 2020-10-03T23:14:39Z

src/magnet_link.rs

+          });
+        }
+        let xt = v
+          .trim_start_matches("urn:bti")


If these aren't present it should return an error.

atomgardner · 2020-10-03T23:59:48Z

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 · 2020-10-04T00:02:41Z

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

casey · 2020-10-04T08:18:56Z

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 · 2020-10-10T10:42:56Z

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

casey · 2020-10-11T00:07:01Z

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

casey

Added a bunch of comments, mostly about error messages and formatting.

casey · 2020-10-12T23:51:17Z

src/error.rs

  #[snafu(display("Invalid glob: {}", source))]
  GlobParse { source: globset::Error },
+  #[snafu(display("Failed to parse hex string `{}`: {}", text, source))]
+  HexParse {


I think that HexParse and InfohashParse should be moved into MagnetLinkParseError. HexParse on its own is not informative enough on the top level for a user to figure out what to do. If eventually multiple parts of the code parse infohashes, InfohashParse can be be moved back into Error. (But actually, if multiple parts of the code are parsing infohashes, then a generic Error::InfohashParse will not be specific enough for the user to know what to do, and will need separate error types.)

I'm thinking how these errors will be reported to the user, and I think in all cases the text that was being parsed should be printed. That way, the user sees the text of the magnet link that failed to parse, in addition to the error.

There are a few ways to do this. One is to have the error be a struct with a kind member that shows the actual error:

struct MagnetLinkParseError { text: String, // magnet link that we attempted to parse kind: MagnetLinkParseKind, // the actual error that occurred } enum MagnetLinkParseKind { Url { source: url::Error}, // … }

Another is to move the text into the Error variant:

enum Error { MagnetLinkParse { text: String, // magnet link we attempted to parse source: MagnetLinkParseError, }, // … }

I think the latter approach probably the right way to go for now. It's less annoying, and if it's eventually insufficient, it can be refactored into the former.

The variant can look like this:

#[snafu(display("Failed to parse magnet link `{}`: {}", text, source))] MagnetLinkParse { text: String, source: MagnetLinkParseError},

I can see why you want MagnetLinkParse to have access to the link URL. But I think it's cleaner to put it inside every variant of MagnetLinkParseError---otherwise, we'd lose the From conversions.

I see what to do 💯

I'm also cool with putting it inside of the MagnetLinkParserError, I was thinking it be annoying.

src/lib.rs

src/infohash.rs

src/magnet_link.rs

casey · 2020-10-13T00:06:18Z

src/magnet_link_parse_error.rs

+    text: String,
+    source: HostPortParseError,
+  },
+  #[snafu(display("Magnet links must begin with `magnet:`"))]


Invalid scheme `{}`, magnet links must use the `magnet:` scheme

Something about the comma looks strange. Sure you want this exact message?

I'm not wedded to the exact message, as long as it contains the invalid scheme. (Since I'm not sure if all users will know what a URL scheme is.)

src/magnet_link_parse_error.rs

casey · 2020-10-13T00:08:22Z

src/magnet_link_parse_error.rs

+
+#[derive(Debug, Snafu)]
+#[snafu(visibility(pub(crate)))]
+pub(crate) enum MagnetLinkParseError {


Some of these messages will be redundant when printed in context, i.e. with Failed to parse magnet link: from the outer Error message.

Tests should be added that use malformed magnet links to trigger all of these error variants, and check that their contents are correct.

casey · 2020-10-17T22:08:29Z

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 · 2020-10-17T22:13:32Z

I just realized a couple things that could be improved:

There's an assert_matches macro that can be used to clean up the tests. You can do assert_matches!(VALUE, PATTERN if CONDITION).
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 · 2020-10-17T23:58:09Z

...to avoid a bunch of context calls.

excellent! 💯 💯

Also, you now have permission to clobber this branch.

type: added

casey · 2020-10-18T00:40:54Z

Sweet, merged! Thank you!

casey requested changes Oct 3, 2020

View reviewed changes

atomgardner force-pushed the magnet-link-parsing branch 2 times, most recently from de37e74 to 0bbb097 Compare October 10, 2020 09:30

atomgardner force-pushed the magnet-link-parsing branch from 0bbb097 to 984e8fe Compare October 11, 2020 00:02

atomgardner force-pushed the magnet-link-parsing branch from 984e8fe to 0b2ae9e Compare October 11, 2020 00:09

atomgardner requested a review from casey October 11, 2020 00:32

atomgardner force-pushed the magnet-link-parsing branch from 0b2ae9e to 80e611d Compare October 11, 2020 05:51

casey requested changes Oct 13, 2020

View reviewed changes

atomgardner force-pushed the magnet-link-parsing branch from 80e611d to bd31ff7 Compare October 15, 2020 10:19

atomgardner requested a review from casey October 16, 2020 09:17

atomgardner force-pushed the magnet-link-parsing branch 2 times, most recently from 316b102 to f354199 Compare October 17, 2020 23:54

Implement FromStr for MagnetLink

97ab785

type: added

casey force-pushed the magnet-link-parsing branch 2 times, most recently from d33fc51 to 97ab785 Compare October 18, 2020 00:27

casey approved these changes Oct 18, 2020

View reviewed changes

casey merged commit 97ab785 into casey:master Oct 18, 2020

atomgardner deleted the magnet-link-parsing branch December 1, 2022 07:42

Add FromStr for MagnetLink #482

Add FromStr for MagnetLink #482

Uh oh!

Conversation

atomgardner commented Oct 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

casey commented Oct 3, 2020

Uh oh!

casey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atomgardner commented Oct 3, 2020

Uh oh!

casey commented Oct 4, 2020

Uh oh!

casey commented Oct 4, 2020

Uh oh!

atomgardner commented Oct 10, 2020

Uh oh!

casey commented Oct 11, 2020

Uh oh!

casey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atomgardner Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

casey commented Oct 17, 2020

Uh oh!

casey commented Oct 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atomgardner commented Oct 17, 2020

Uh oh!

casey commented Oct 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

atomgardner commented Oct 3, 2020 •

edited

Loading

atomgardner Oct 14, 2020 •

edited

Loading

casey commented Oct 17, 2020 •

edited

Loading