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
New: if search results empty, use GetMovieByImdbId #9810
base: develop
Are you sure you want to change the base?
New: if search results empty, use GetMovieByImdbId #9810
Conversation
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.
on 427 already does an IMDb search..? Why is this change needed? To not need the prefix?
Should this be done for tmdb too?
should i go continue the PR? |
// if output is zero and format is ttXXXXXXXX, use GetMovieByImdbId | ||
if (output.Count == 0 && title.Length == 10 && title.StartsWith("tt")) | ||
{ | ||
var imdbid = title; | ||
try | ||
{ | ||
var movieLookup = GetMovieByImdbId(imdbid); | ||
return new List<Movie> | ||
{ | ||
new Movie { MovieMetadata = movieLookup } | ||
}; | ||
} | ||
catch (MovieNotFoundException) | ||
{ | ||
return new List<Movie>(); | ||
} | ||
} | ||
|
||
return output; |
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.
Hey, sounds good.
Just like #9754, I would rather prefer for this to happen in the MovieLookupController
.
We like documented code, but that comment can be removed. output
is somewhat confusing, and I think we should check if StartsWith("tt")
and ^tt\d{7,8}$
.
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.
ok got it. i'll edit it
thank you for the review
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.
need advise,
should i move the main logic to SkyHookProxy.cs
?
INamingConfigService namingService, | ||
IMapCoversToLocal coverMapper, | ||
IConfigService configService) | ||
IProvideMovieInfo movieInfo, |
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.
why are we reformatting?
@@ -49,7 +51,9 @@ public object SearchByTmdbId(int tmdbId) | |||
{ | |||
var availDelay = _configService.AvailabilityDelay; | |||
var result = new Movie { MovieMetadata = _movieInfo.GetMovieInfo(tmdbId).Item1 }; | |||
var translation = result.MovieMetadata.Value.Translations.FirstOrDefault(t => t.Language == (Language)_configService.MovieInfoLanguage); | |||
var translation = |
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.
why are we reformatting?
@@ -59,18 +63,40 @@ public object SearchByImdbId(string imdbId) | |||
var result = new Movie { MovieMetadata = _movieInfo.GetMovieByImdbId(imdbId) }; | |||
|
|||
var availDelay = _configService.AvailabilityDelay; | |||
var translation = result.MovieMetadata.Value.Translations.FirstOrDefault(t => t.Language == (Language)_configService.MovieInfoLanguage); | |||
var translation = |
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.
why are we reformatting?
@@ -79,12 +105,14 @@ private IEnumerable<MovieResource> MapToResource(IEnumerable<Movie> movies) | |||
|
|||
foreach (var currentMovie in movies) | |||
{ | |||
var translation = currentMovie.MovieMetadata.Value.Translations.FirstOrDefault(t => t.Language == movieInfoLanguage); | |||
var translation = |
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.
why are we reformatting?
var resource = currentMovie.ToResource(availDelay, translation); | ||
|
||
_coverMapper.ConvertToLocalUrls(resource.Id, resource.Images); | ||
|
||
var poster = currentMovie.MovieMetadata.Value.Images.FirstOrDefault(c => c.CoverType == MediaCoverTypes.Poster); | ||
var poster = |
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.
why are we reformatting?
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.
Ups, this is unintended.
I'm gonna revert it
I think maybe automatic from the jetbrain IDE. Apologize for these one
This reverts commit a1816c6.
ready for review
|
Build fails..? |
fixed. |
Don't worry about that one, it's very flakey. |
Database Migration
NO
Description
Implement find by imdb id if search result is empty
Note: This is my first PR on this repo.
I'm aware that the issue is still on "Need Triage Status"
feel free to drop the PR. alternative suggestions are welcome.
Screenshot (if UI related)
NO
Todos
Issues Fixed or Closed by this PR