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

add support for new DownloadStation API #9555

Merged
merged 6 commits into from
May 31, 2021
Merged

add support for new DownloadStation API #9555

merged 6 commits into from
May 31, 2021

Conversation

BenjV
Copy link
Contributor

@BenjV BenjV commented May 18, 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

Ow you did a new pr. Good.

medusa/clients/torrent/downloadstation.py Outdated Show resolved Hide resolved
medusa/clients/torrent/downloadstation.py Outdated Show resolved Hide resolved
medusa/clients/torrent/downloadstation.py Outdated Show resolved Hide resolved
medusa/clients/torrent/generic.py Outdated Show resolved Hide resolved
medusa/clients/torrent/generic.py Outdated Show resolved Hide resolved
medusa/clients/torrent/generic.py Show resolved Hide resolved
medusa/clients/torrent/generic.py Show resolved Hide resolved
medusa/clients/torrent/generic.py Outdated Show resolved Hide resolved
medusa/clients/torrent/generic.py Show resolved Hide resolved
medusa/clients/torrent/generic.py Show resolved Hide resolved
@p0psicles
Copy link
Contributor

p0psicles commented May 21, 2021

I did a review on generic.py first. As this is the more critical module of the two, as it's used by all the supported torrent clients. We need to make sure that all clients work as expected with these changes. Just from reading all these code changes in it, I'm not 100% sure it will. So the only way to know is to test it.

If you want to go through with the refactor to generic, I suggest you use docker to run them. Maybe this can be of help. When I have more time I'll go through the downloadstation.py changes. Although those looked okay at first sight. Except from the code styling and linting issues.

@BenjV
Copy link
Contributor Author

BenjV commented May 21, 2021

Code styling is a a matter of opinion, so feel free to change it.
Be careful about changing the parameter dict, because as I said they are the tricky part of the DownloadStation api.

I use a lot of f-string formatting instead of the old %s and/or .format.
My opinion is that they are much more readable then the old style, but you have to be careful with the quotes and the double quote.
It is not always possible to use the f'' or f"" because if the variables or string contains one of them, you need to switch to the other for the outer quotes
For example:
f" This is "my" text" does not work and should be changed in f' This is "my" work', so especially working with json formatted string or dict you must be careful.

@p0psicles
Copy link
Contributor

p0psicles commented May 28, 2021

Can you give me access to your forks branch? Then i'm okay with making some changes to it. And then you can also test them against the downloadstation's api.

Alternative is that you make changes that are required to pass the build process, and I make the other changes after merged. But currently it's not building.

Btw sorry for my late response. I've had a busy week.

@BenjV
Copy link
Contributor Author

BenjV commented May 28, 2021

I have send you an invite for my fork.
You can change anything you like, but please do not change the things that are needed for DownLoad Station like the params dict.
I you do that, things will not function any longer.

And by the way, if I look at the reasons why the test are failing, non of those reasons are in the the two modules (generic.py and downloadstation.py) that I changed.

@p0psicles
Copy link
Contributor

You can ignore the api test. But the lint test in backend test is failing because of changes in this pr.

* Use BraceAdapter formatting
* Use single quotes for f-strings
* Fixed docstrings formatting
* Bad newlines
* Unused imports / import ordering
@p0psicles
Copy link
Contributor

I accidentally pushed to the wrong branch name master-benjv. Sorry for that.

I pushed only a commit that fixes the lining issues. Next is a small refactor to the send_torrent() exception handling part.
And then last we can test the json.dumps() method. Let me know if you willing to test that one out for me?

@BenjV
Copy link
Contributor Author

BenjV commented May 30, 2021

No the jason dump methode does not work, it will convert also things that must not be converted to json.
I know it is strange, but that's the way Synology created that api and changing it is not possible.

Don't be so stubborn and just accept my correctly working solution.
I have spent a fair amount of time testing this and what I created is working fine.

@p0psicles
Copy link
Contributor

p0psicles commented May 30, 2021

I'm not asking for the change because I want to be stubborn. I'm sorry if I gave you that impression.
But i'm asking for it because i'd prefer to add maintainable code to our project.

For example. What if you'd loose interest in supporting the downloadstation api in 2 year.
And someone else wants to extend the functionaly, like add new downloadstation api routes. They would need to trial&error like you had to. And while you managed. It's not sure if someone else would. Wouldn't it be great if we could just use basic dicts with base types like, boolean, string, integers. And not have to worry about wrapping some strings in quotes and some not?

If you already tested my proposal before, fine. If not, please consider. If you really don't want to, fine i'll merge as is.

@BenjV
Copy link
Contributor Author

BenjV commented May 30, 2021

One of the first things I tested long before you asked it, was your solution with json dumps, but that does not work!

Only some of the data fields must be json coded not the whole dict, if you do the whole dict it does not work.
The only solution is to add quotes to those fields who need it and leave the rest alone.
I got this information directly from a support engineer of Synology, because there is no documentation (yet) for this API and the old API does now only works for links and not any more for torrent files.

Of course there are more ways than f-string to add quotes to those fields, but that would make in even less readable.
And precisely for the reason of maintainability I added extensive comment to the code why I did it that way.

By the way this API also works for NZB's so if you want I could write an NZB client for Download Station.

@p0psicles
Copy link
Contributor

Tnx for your explanation and your patience.
Sure, let's keep it as is.
Just need to do a small change. And then I'll merge.

Let's park the NZB provider for now. As that would require some more frontend changes. And I don't have much time now

* No need to initialize result.hash. As this attribute is always available.
@p0psicles p0psicles merged commit 08ee9f1 into pymedusa:develop May 31, 2021
@reconman
Copy link
Contributor

reconman commented May 31, 2021

Since the latest develop commit, sending torrents to the Transmission RPC url causes HTTP 409 errors. Haven't been able to find out any details yet.

Something in this PR must have caused it. Reverting back to master fixed the problem.

@p0psicles
Copy link
Contributor

p0psicles commented Jun 1, 2021

@reconman tnx for reporting, i'll look into it asap
Will be fixed with PR #9598

Edit.. Develop should be fixed

@p0psicles
Copy link
Contributor

@reconman can you confirm that the issue is fixed for you?

@reconman
Copy link
Contributor

reconman commented Jun 3, 2021

Seems fixed, torrents can be transmitted to transmission again. Thank you.

@BenjV
Copy link
Contributor Author

BenjV commented Jun 4, 2021

@p0psicles
I see that on the develop branch you removed the call to self._get_auth() in the init.
Downloadstation.py needs this call for the correct function.
So on the current develop branch downloadstation does not function.

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.

3 participants