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

Feature/downloadstation torrent path #9482

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

p0psicles
Copy link
Contributor

@p0psicles p0psicles commented Apr 26, 2021

  • PR is based on the DEVELOP branch
  • Don't send big changes all at once. Split up big PRs into multiple smaller PRs that are easier to manage and review
  • Read the contribution guide

@p0psicles
Copy link
Contributor Author

@BenjV i've also send invites to join the developer team, which will give you write access to the branch.

@BenjV
Copy link
Contributor

BenjV commented Apr 28, 2021

@p0psicles
I did not get any invite to get acces to this branch
I tested your changes and they worked fine.

A small remark, why are parameters send to all the torrent client?
That information is copied in in the init into object variables (host -> self.host, username -> self.username, etc) and all self.... variable are accessible in all the clients.
Those parameters are never used and so the are useless.
Why not keep it simple and remove them?

@p0psicles
Copy link
Contributor Author

p0psicles commented Apr 28, 2021

A small remark, why are parameters send to all the torrent client?
What do you mean by this?

Only one torrent client class is used at a time. And it needs to initialize the object with the params, to pass username, password, host (and now torrent_path) into it.

The downside of this approach, is that you need to recreate a new object each time. Opposed to initialize an object and put it on the app.* at startup, and then pass your username, password, etc params through the objects methods. Which is how it's done in other parts of the application. But for now, it is, what it is.

I did not get any invite to get acces to this branch

That's weird. I'm sure i've invited you to the https://github.com/pymedusa/Medusa project. That should give you write access to branches. The invite should be in your main that's attached to your github account. Or maybe in your github inbox?

@BenjV
Copy link
Contributor

BenjV commented Apr 28, 2021

What is meant is that the "GenericClient" gets the information via the parameters and stores them as object data (for example self.username).
Those variables are accessible to the torrent client in use (just like self.url and self.response), because those client are not used as and real object, so they share the data of the generic client object.
So is has no use to put them on the parameter list of the init of the DownloadStationAPI
They also are never used in any of the torrent client.

Also I stumbled on another issue.
In the user interface there is a field "SSL CA Bundle", where you can put the location of a certificate.
If that field is left empty, then the app.TORRENT_VERIFY_CERT has the value "False"
In the user interface it stateds that if left empty the certificate from certifi is used.

So somewhere that should be done with a statement like this.
app.TORRENT_VERIFY_CERT = certifi.where()

And I looked again but still no invite or access rights.

…wnload-station-torrent-path

# Conflicts:
#	medusa/clients/torrent/downloadstation.py
#	medusa/clients/torrent/generic.py
@p0psicles
Copy link
Contributor Author

p0psicles commented May 31, 2021

because those client are not used as and real object, so they share the data of the generic client object.
I'm still not sure why you think they are not used as real objects. If you look at this line:
https://github.com/pymedusa/Medusa/blob/master/medusa/process_tv.py#L962

You'll see that client = torrent.get_client_class(app.TORRENT_METHOD)() will create an object from the class (for ex) DownloadStationAPI, which in turn is subclassed from GenericClient.

I know that it's not the most readable construction. But it is using objects.

In the user interface there is a field "SSL CA Bundle", where you can put the location of a certificate.
If that field is left empty, then the app.TORRENT_VERIFY_CERT has the value "False"
In the user interface it stateds that if left empty the certificate from certifi is used.

I had to double check. But from this part of the code: https://github.com/pymedusa/Medusa/blob/master/medusa/session/core.py#L66

If SSL_CA_BUNDLE is empty but SSL_VERIFY is true, it will by default use certify.where().
That should play out nicely with the change i made to your branch.

…wnload-station-torrent-path

# Conflicts:
#	medusa/clients/torrent/rtorrent.py
@p0psicles
Copy link
Contributor Author

@BenjV Does the torrent_path verification work with this branch? Then i'd like to merge it.

@p0psicles
Copy link
Contributor Author

@BenjV also maybe it's a good idea to look at the download_handler for the downloadstation. I don't think users would benifit a lot of a downloadstation nzb api. As they can always make use of the black hole (nzb drop) functionality. But for torrents the torrent api has some additional benifits when using the downloadhandler. As for ex. it can track the upload/download ratio, and then perform actions like, deleting the torrent, or pausing it.
So it would be nice if users also can use downloadstation with the downloadhandler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants