Perfect your code
With built-in code review tools, GitHub makes it easy to raise the quality bar before you ship. Join the 40 million developers who've merged over 200 million pull requests.
Sign up for free See pricing for teams and enterprisesImplements BEP53 to allow file selection using &so in magnetURIs #1396
Conversation
This comment has been minimized.
This comment has been minimized.
welcome
bot
commented
May 18, 2018
|
Thanks for opening this pull request! Please check out our contributing guidelines. |
|
Thanks for the PR. Looks really good! I shared some feedback inline. Once you've addressed it, we can merge it and release a new version :) |
| if (self.so) { | ||
| var specificFiles = parseRange.parse(self.so) | ||
| // https://github.com/webtorrent/webtorrent/issues/164 | ||
| self.deselect(0, self.pieces.length - 1, false) |
This comment has been minimized.
This comment has been minimized.
feross
May 18, 2018
Member
I believe that this line should not be necessary since you moved the self.select(0, self.pieces.length - 1, false) call to an else block below. So, what is this actually deselecting?
This comment has been minimized.
This comment has been minimized.
SilentBot1
May 18, 2018
Author
Member
Initially I had the if (self.so) statement under self.select all without the else statement, this is leftover code from that which can be removed, removed in next commit.
| @@ -509,6 +505,22 @@ Torrent.prototype._onMetadata = function (metadata) { | |||
| return new File(self, file) | |||
| }) | |||
|
|
|||
| // Only select specified files (BEP53) | |||
| if (self.so) { | |||
| var specificFiles = parseRange.parse(self.so) | |||
This comment has been minimized.
This comment has been minimized.
feross
May 18, 2018
Member
Can we name this variable selectOnlyFiles, since "so" stands for "select only"?
This comment has been minimized.
This comment has been minimized.
SilentBot1
May 18, 2018
Author
Member
Of course, that would also make it more clear for people searching the code.
| @@ -509,6 +505,22 @@ Torrent.prototype._onMetadata = function (metadata) { | |||
| return new File(self, file) | |||
| }) | |||
|
|
|||
| // Only select specified files (BEP53) | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| self.deselect(0, self.pieces.length - 1, false) | ||
|
|
||
| self.files.forEach(function (v, i) { | ||
| specificFiles.indexOf(i) === -1 ? self.files[i].deselect() : self.files[i].select(true) |
This comment has been minimized.
This comment has been minimized.
feross
May 18, 2018
Member
Unless I'm missing something, it shouldn't be necessary to call deselect() since all files start out deselected. Can we change this to just call select()?
Also, can you using array.includes() instead of array.indexOf() since it's clearer?
This comment has been minimized.
This comment has been minimized.
SilentBot1
May 18, 2018
Author
Member
You're correct, the self.files[i].deselect() serves no purpose as pieces start out deselected, this will be removed in the next commit.
Replacing specificFiles.indexOf(i) with selectOnlyFiles.includes(i) for clarity will also be in the next commit.
Thank you for the review :)
This comment has been minimized.
This comment has been minimized.
welcome
bot
commented
May 18, 2018
|
Congrats on getting your first pull request landed! |
This comment has been minimized.
This comment has been minimized.
|
@SilentBot1 Thanks for making the changes and merging. Released as 0.100.0. |
SilentBot1 commentedMay 18, 2018
This pull request implements BEP53 as requested in #1395.
If
&so=is specified, all pieces are deselected and only the pieces which contain parts of the selected files are downloaded, if no pieces are specified, the entire file is left deselected.The use of parse-numeric-range allows for ranges to be specified in three different formats which include using hyphens, which is what the BEP calls for, e.g.
1-5which is equal to[1, 2, 3, 4, 5], using two dots e.g.1..5which is also equal to[1, 2, 3, 4, 5]or by using three dots e.g.1...5which would equal[1, 2, 3, 4].Any feedback or suggestions are welcome.