-
-
Notifications
You must be signed in to change notification settings - Fork 70
Refactor language component and add primary language support to tvdb provider #365
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
base: master
Are you sure you want to change the base?
Refactor language component and add primary language support to tvdb provider #365
Conversation
|
looks good! 🎉 |
|
Here it is!:) |
maxdorninger
left a comment
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.
While this generally looks good, there are some things which need to be changed or further explained/documented.
- Why was the
languageparameter renamed tooriginal_language, this new name doesn't really make sense. - Also the multi language aspect of the TVDB provider doesn't really work? I mean that it still is always going to get the metadata in English.
I think it'd be better if this were 2 PRs, one for the refactoring of the TMDB provider and one for adding the language feature to the tvdb provider.
| @@ -290,9 +362,10 @@ def get_movie_metadata( | |||
|
|
|||
| return Movie( | |||
| name=movie["name"], | |||
| overview="Overviews are not supported with TVDB", | |||
| overview=movie.get("overview", "No overview available"), | |||
| year=movie.get("year"), | |||
| external_id=movie["id"], | |||
| metadata_provider=self.name, | |||
| imdb_id=imdb_id, | |||
| original_language=original_language, | |||
| ) | |||
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'm a bit confused on what the point of the original_language argument is in this function.
It will still get the English metadata from TVDB, right?
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.
Since tvdb provider is still only making plain queries, yes you are right, it is not being used for searching and fetching metadata. Still it serves to add original language to the db record for the show/movie which is consistent with what we already do with tmdb.
| if original_language: | ||
| return self.__make_tmdb_request( | ||
| endpoint=f"/tv/shows/{show_id}", | ||
| original_language=original_language, | ||
| context=f"get show metadata for ID {show_id}", | ||
| ) | ||
| raise | ||
| return initial_metadata |
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 original_language: | |
| return self.__make_tmdb_request( | |
| endpoint=f"/tv/shows/{show_id}", | |
| original_language=original_language, | |
| context=f"get show metadata for ID {show_id}", | |
| ) | |
| raise | |
| return initial_metadata | |
| if not original_language: | |
| return initial_metadata |
|
Sorry for making the PR so broad.
Maybe that is might as well and this PR can be limited to refactoring. The multi language feature for tvdb can be done separately, or what do you think is best? |
Make the language parameter names more clear. Creates a unified method
__make_tmdb_request.It also adds two minor fixes: missing . in metadataProvider/utils.py and make a return value
Nonein movies/router.pyIf this looks fine I can adjust tvdb.py in the same way and add multi-language support.