Add support for BTv2 magnet links #72
Conversation
|
Hey thanks for this, I'm traveling with almost no internet and only a phone, so will take a look in a few days when I come back. |
jabedude
force-pushed
the
bep52-magnet-support
branch
from
011ec98 to
ed872f6
Compare
|
@jabedude left a few pretty significant design comments |
jabedude
force-pushed
the
bep52-magnet-support
branch
from
ed872f6 to
091aef5
Compare
|
Code LGTM so far, but:
|
jabedude
force-pushed
the
bep52-magnet-support
branch
from
091aef5 to
0836b65
Compare
|
Tests fixed, changed |
|
|
||
| fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result { | ||
| write!(formatter, "a 20 byte slice or a 40 byte string") | ||
| formatter.write_str("a byte array of length ") |
There was a problem hiding this comment.
nit: could have just done formatter.write_fmt(format_args!("a byte array of length {}", N)))
| match info_hash { | ||
| Some(info_hash) => Ok(Magnet { | ||
| info_hash, | ||
| match info_hash_found { |
There was a problem hiding this comment.
nit: you don't need a separate variable to track this, could have just check if at least one of id20 and id32 is set
| self.info_hash.as_string(), | ||
| self.trackers.join("&tr=") | ||
| ) | ||
| if let (Some(id20), Some(id32)) = (self.id20, self.id32) { |
There was a problem hiding this comment.
nit: it's a bit repetitive to handle all cases like this, you could have done this without a big branch, but small branches instead:
write!(f, "magnet:?")?;
if let Some(id20) = self.id20 {
write!("&...");
}
if let Some(id32) = self.i32 {
write!("&...");
}
write!("&tr={}", self.trackers.join("&tr="))?;
| id32.as_string(), | ||
| self.trackers.join("&tr=") | ||
| ) | ||
| } else { |
There was a problem hiding this comment.
please don't panic in Display trait. If you restructure the code like I suggested above, let it just print a magnet without hashes
|
All comments are minor, we can fix them later, so merging |
This is a tiny step to supporting BEP52 (#70). Ideally the new
InfoHashtype would replaceId20inMagnet, but that would be a significant refactor across the project and it's not required until we actually begin performing lookups with SHA-256 hashes