Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added pause/resume, refactored code, improved command line interface (and more) #515

Closed
wants to merge 31 commits into from

Conversation

@whitef0x0
Copy link

whitef0x0 commented Dec 5, 2015

This is a big PR, so let me just list out everything that I've changed/added

New Features

Torrent.js

  1. Added pause() & resume() for active torrents
    • pauses and resumes the current torrent
  2. Added disableSeeding() and enableSeeding()
    • enables or disables autoseeding

Index.js

  1. Added pause() & resume()
    • pauses and resumes a client's torrent (calls Torrent.pause/resume)

bin/cmd.js

  1. Added CLI menu to quit, resume and pause while seeding or downloading a torrent

New Testing

test/browserAppendTo

  1. Added a browser-based video streaming test for AppendTO

test/Resume-Torrent-Scenarios

  1. Added 5 scenario-based test cases for pause and resume (resume-torrent-scenarios.js)

Torrent.js

  1. Added unit tests for private functions in Torrent.js

I also added coveralls support and code coverage with istanbul if you choose to use it and I fixed formatting errors in the .travis.yml.

@rom1504

This comment has been minimized.

Copy link
Member

rom1504 commented Dec 5, 2015

I don't know about the rest, but I bet searching on kat is probably not okay to be in webtorrent and should be in an other repo that use webtorrent ;)

whitef0x0 added 2 commits Dec 5, 2015
@whitef0x0

This comment has been minimized.

Copy link
Author

whitef0x0 commented Dec 5, 2015

I'll go ahead and remove the search.

@whitef0x0

This comment has been minimized.

Copy link
Author

whitef0x0 commented Dec 5, 2015

I've removed the search functionality for now.

@whitef0x0

This comment has been minimized.

Copy link
Author

whitef0x0 commented Dec 6, 2015

@feross have you been able to take a look at this?

@whitef0x0

This comment has been minimized.

Copy link
Author

whitef0x0 commented Dec 11, 2015

I've also removed the CLI part for test.js.

Is there anything else I should do @DiegoRBaquero

@DiegoRBaquero

This comment has been minimized.

Copy link
Member

DiegoRBaquero commented Dec 11, 2015

@whitef0x0 This PR is for @feross to decide. (I don't know what he is up to though because he hasn't been around the last few days)

In my opinion, I don't like disabling seeding option. BitTorrent is about sharing.

@niieani

This comment has been minimized.

Copy link
Contributor

niieani commented Jan 15, 2016

@DiegoRBaquero there are legitimate use cases for disabling seeding. BitTorrent is not only about end-user sharing, it's a technology for transferring data that can also be used by companies / enterprises, e.g. in low-end IoT devices that can't spare the power/connectivity required to seed.

@rom1504

This comment has been minimized.

Copy link
Member

rom1504 commented Mar 11, 2016

I think this would be better split into several PRs to make it easier to review it.
Also a lot of this can now move to a webtorrent-cli PR

@feross

This comment has been minimized.

Copy link
Member

feross commented Mar 19, 2016

I appreciate that this PR must have taken a lot of effort to put together, but there are lots of problems with it.

  • Each change should be sent as a separate PR so it can be reviewed independently. Giant PRs are overwhelming to review and have to be accepted all-or-nothing.
  • In the future, you should probably create an issue to discuss the features you want to add before doing a bunch of work. Because features like these are a no-go:
    • Hardcoding urls to external services. WebTorrent is an un-opinionated torrent client and we're not ever going to include third-party search.
    • Disabling web seeding is not a use case that we're going to support out of the box. In the future, we hope to allow custom "strategies" in addition to the built in ones: sequential and rarest-first. That's the place where a user could disable seeding if they need it for IoT.
  • No bower.json. Bower is an anti-pattern.
  • Don't update the minified build – that's done at release time.
@feross feross closed this Mar 19, 2016
@lock

This comment has been minimized.

Copy link

lock bot commented May 4, 2018

This thread has been automatically locked because it has not had recent activity. To discuss futher, please open a new issue.

@lock lock bot locked as resolved and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.