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 of IMDBid search for ncore indexer #7847

Merged
merged 5 commits into from
Apr 17, 2020

Conversation

morpheus133
Copy link
Contributor

Hi!
I already have some solution for #7822
Add support for both TV and Movie search with imdbid for ncore.

@RoloSoze
Copy link
Collaborator

Any reason why this hasn't been approved yet?

@cadatoiva
Copy link
Collaborator

For me, I've been too busy to be thorough. Also, on initial glance this PR doesn't follow our code style for new code. I now have an account to test changes directly, and there's more features that need implementing/testing as well. I appreciate the head start on this feature, and I'll be using it as the basis for the full update of this tracker.

@ngosang
Copy link
Member

ngosang commented Mar 28, 2020

@RoloSoze you can help us by testing the PRs. Since you have accounts in many sites, you can download the release generated in the PR and perform some kind of QA. We can't approve changes in the code without some testing. I don't have account in this site.

  1. Click in the checks tab
    image
  2. Select the first element in le list "Jackett" and Click View more details in Azure pipeline
    image
  3. Scroll down. Click in Create binaries => 1 artifact
    image
  4. Download the drop folder. You will find the Jackett executable there.
    image

@RoloSoze
Copy link
Collaborator

@ngosang I can do that for other PRs when needed, sure. For this one, @cadatoiva said he wants to work on the code himself since he now has access to nCore. Do you think it's better to wait for him to do that?

@ngosang
Copy link
Member

ngosang commented Mar 28, 2020

I think it's best to test the PR after the @cadatoiva changes.

@RoloSoze
Copy link
Collaborator

I think it's best to test the PR after the @cadatoiva changes.

Yeah I agree.

@morpheus133
Copy link
Contributor Author

@cadatoiva if you dont have time for it I may can do the refactor.
I only need an existing indexer what was already refactored and I can use as a starting point.

@cadatoiva
Copy link
Collaborator

Sorry, I was waiting for a specific PR to be merged that would make refactoring this one better match our other trackers. I'll have it done today.

@cadatoiva
Copy link
Collaborator

Ok, that took longer to get done than I was expecting. This tracker is now refactored to best match our style guide, minus one area which now has a TODO in it.

I have tested it, but @morpheus133 if you could also download and test to verify my changes, I'd appreciate it.

@ngosang if you wouldn't mind doing a review of the code so someone independent of the changes puts eyes on it, I'd similarly appreciate. This one has multi-page processing in it, similar to TvStore (#7978).

src/Jackett.Common/Indexers/NCore.cs Outdated Show resolved Hide resolved
src/Jackett.Common/Indexers/NCore.cs Outdated Show resolved Hide resolved
src/Jackett.Common/Indexers/NCore.cs Outdated Show resolved Hide resolved
src/Jackett.Common/Indexers/NCore.cs Outdated Show resolved Hide resolved
@cadatoiva cadatoiva merged commit cd4e5de into Jackett:master Apr 17, 2020
@RoloSoze
Copy link
Collaborator

I can confirm that the IMDb ID search works well. Everything else I tested works well, too. #4859 is looking great right now :) Thanks, guys @morpheus133 @cadatoiva

@morpheus133
Copy link
Contributor Author

@ngosang one question:
ImdbTVSearch is working correctly with this implementation, why it is disabled?

@cadatoiva
Copy link
Collaborator

@morpheus133 in Sonarr v3 when a tracker supports ImdbTVSearch, the search it sends is imdb=<show level imdb>&season=##&episode=## while the imdb it sends is the one that NCore uses, there is no way to send nCore the season and episode details to filter the results (that I saw).

And while we can filter by season/episode before sending the results, it would mess with the logic involved in using offset and limit parameters used in the multi-page processing. (we can't just assume that offset of 75 on 25 results per page means start on page 3, because with filtering we'd duplicate results).

The ImdbTVSearch can still be used, and it will still work on manual searches. However, because all current options for implementation, we are not telling other programs (like Sonarr v3) that we support the type of search.

@morpheus133
Copy link
Contributor Author

Hi!
With my original implementation for this request (imdb=&season=##&episode=##) will give only the requested season episode to sonarr v3, or nothing if not founded.

But yes it can cause problems when using offsets.
But the basic config for sonar is limit 100 so it can cause problem only when more than 100 result is there for an series/episode. (and it is almost never happen.)

@kevindd992002
Copy link

Anybody?

@RoloSoze
Copy link
Collaborator

Anybody?

@kevindd992002 IMDb ID search has already been implemented. Upgrade to the latest version of Jackett... Unless you had another question?

@kevindd992002
Copy link

It was turned off a couple of weeks ago because of how it was implemented incorrectly, right? So it was fixed already?

@cadatoiva
Copy link
Collaborator

What was turned off was Sonarr v3 using IMDB id to search for TV episodes. IMDB searching in general is still available in all other cases.

@kevindd992002
Copy link

Sorry! I was on my phone and had two github issues opened and I accidentally posted my follow-up reply here. This is what I was actually trying to follow-up:

#8107

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.

None yet

5 participants