-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[EZTVBridge] Switch to using EZTV API #2476
Conversation
One additional feature: Filters for torrent resolutions. What is the process for getting a pull request accepted? I now have three pull requests in progress, and I'm getting a bit concerned when seeing other open pull requests from a year or two ago. |
bridges/EZTVBridge.php
Outdated
'ids' => array( | ||
'name' => 'Show IMDB IDs', | ||
'exampleValue' => 'showID1,showID2,…', | ||
'required' => true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Old feeds:
- use 'i' param as id
- do not use it as IMDB ID.
When bridge is updated, old feed links will be broken. It is important to safe backward compatibility if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only way to make it backwards compatible would be to keep all the previous HTML-parsing code in the Bridge, and have two separate input fields. One for EZTV show ID, which triggers usage of the HTML-parsing code, and one for IMDB show ID, which triggers usage of the API-parsing code.
Would it be preferable to submit this as a new Bridge? EZTVApiBridge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think making a new bridge is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has come up a few times in the past.
The bridge would be enhanced by changing from scraping to api usage but will lose compatibility. I think we should come up with a general approach.
As much as I dislike breaking backwards compatibility, I would hate duplicate bridges even more. Different capabillities, different outputs, different usage and all for the same site. It also creates 2 maintainers and there will be requests for feature parity anyways ("the other eztv bridge can filter for X, I want that for this eztv bridge too").
So, my vote is for changing to the API and breaking backwards compatibility if we have a technology switch from scraping to api. In general and for this bridge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we break BC I suggest we detect old feed urls and then serve a single feed item letting the user know that they should update the feed url. Could even link directly to the bridge form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldnt the old URL throw an error that reaches the feed reader anyways?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the feed reader would probably let the user know that the feed is broken if we don't serve http 200.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So one solution is to keep old context but hide it in the web UI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
? No.. just nuke the old version, let the error handling of the rss reader handle the notification that the user needs to generate a new feed.
Everything else is either duplication or will create a mess in the bridges and make them unreadable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im fine with nuking the old version.
Hi, After the discussion, we will do the cut in backwards compatibility in order to use the new and enhanced functionality. One other change: please change the examplevalues to an actual show ID and put explanations in the 'title' parameter. Thanks |
This is done in 293b4f2 |
🤖 Pull request artifacts
|
Top. Can you resolve the conflicts? |
Do you mean me? How do I do that? I'm a github novice. I clicked the "Resolve conflicts" button that appears on this page, but I don't see anything in the next page that allows me to resolve the conflict. |
Fixed it for you. For the next time: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-on-github The page you saw gave you both options that caused the merge conflict. You basically remove whatever you dont want in your final commit (so i just deleted the old code part and removed the conflict markers, then saved it). the gh ui is pretty cool for smaller stuff like this. Thanks for this PR! |
Instead of parsing HTML, use the available public EZTV API and get the data from the JSON response.
Benefits/changes: