vfalies / tmdb

PHP Wrapper for TMDB (The Movie DataBase) API V3
https://vfac.fr/projects/tmdb
MIT License
22 stars 5 forks source link

Add Logo Result, Name Result, and TVNetwork item #41

Closed srichter closed 4 years ago

srichter commented 4 years ago

This adds the following:

I've included tests for each as well. The only "non-standard" method I added was getBestFilePath() on a Logo result. The API always returns a .png file path, but for some Logos there is an .svg available which is indicated in the file_type field. This method checks for the existence of an svg image and returns that, otherwise returns the png.

srichter commented 4 years ago

Odd, not sure why the tests failed there, they all pass locally.

srichter commented 4 years ago

In complement of my comments, tests code on Items\TVNetwork missing:

I assume the non-covered lines it is complaining about are tests for when the API would be returning an "empty" search results list (for instance such as companyEmptyOk.json). Since I didn't see a way to search for Networks with the API, I'm not sure we'll ever have empty results like that. What I did was find one where it returns empty strings for each attribute, but that doesn't seem to satisfy the code-coverage since the properties will still be set on the object and the return ''; will never run. Maybe the solution is to remove those checks in the getters, since if we call for a Network that doesn't exist it'll throw an API exception instead of returning an empty object. What do you think?

srichter commented 4 years ago

Also, how do you want me to handle the @author tag? Obviously I am reusing a lot of your code here. Do you want me to also add an @author tag for you on the class/method-level or is the one in the header sufficient?

edit: Will add the new commits once your test-fixing commits are merged.

vfalies commented 4 years ago

Also, how do you want me to handle the @author tag? Obviously I am reusing a lot of your code here. Do you want me to also add an @author tag for you on the class/method-level or is the one in the header sufficient?

edit: Will add the new commits once your test-fixing commits are merged.

I think that if you create a new method/class you must mentionned only your name. If you complete an existant class / method, you add you name in comment. My name is at the beginning of each files to specify the package it is sufficient.

vfalies commented 4 years ago

In complement of my comments, tests code on Items\TVNetwork missing:

I assume the non-covered lines it is complaining about are tests for when the API would be returning an "empty" search results list (for instance such as companyEmptyOk.json). Since I didn't see a way to search for Networks with the API, I'm not sure we'll ever have empty results like that. What I did was find one where it returns empty strings for each attribute, but that doesn't seem to satisfy the code-coverage since the properties will still be set on the object and the return ''; will never run. Maybe the solution is to remove those checks in the getters, since if we call for a Network that doesn't exist it'll throw an API exception instead of returning an empty object. What do you think?

You should not remove these tests in the getters because, if you check the documentation all response fields are optional !

image

In consequence, possibly you can have a valid response (200) with no information inside. You must simulate these response in your json response to test this.

Don't forget Murphy's law : If anything can go wrong, it will. :)