Skip to content

Conversation

ptheofan
Copy link
Contributor

Patch for Issue #242

@codecov-commenter
Copy link

codecov-commenter commented Sep 24, 2021

Codecov Report

Merging #243 (d0efad7) into 4.0 (e330e8f) will decrease coverage by 0.01%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##                4.0     #243      +/-   ##
============================================
- Coverage     84.45%   84.43%   -0.02%     
- Complexity     2166     2167       +1     
============================================
  Files           196      196              
  Lines          5454     5456       +2     
============================================
+ Hits           4606     4607       +1     
- Misses          848      849       +1     
Impacted Files Coverage Δ
lib/Tmdb/Model/Tv/Episode.php 93.75% <66.66%> (-0.94%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e330e8f...d0efad7. Read the comment docs.

Comment on lines 167 to 169
if ($airDate === '') {
$airDate = null;
} elseif (!$airDate instanceof DateTime && $airDate !== null) {
Copy link
Member

@wtfzdotnet wtfzdotnet Sep 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ptheofan think we are better off changing the check to not empty ( and we could definitively still be more defensive ), would you mind checking if this might occur in different models?

Suggested change
if ($airDate === '') {
$airDate = null;
} elseif (!$airDate instanceof DateTime && $airDate !== null) {
} elseif (!$airDate instanceof DateTime && !empty($airDate)) {

Copy link
Contributor Author

@ptheofan ptheofan Sep 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't agree more. Didn't go with empty in the first place as a lot of people find it to be tricky for junior devs. :)

Similar PR for the movie model #244

simplified check using empty and reduced complexity of instanceof Datetime
@wtfzdotnet wtfzdotnet merged commit 89a3c9a into php-tmdb:4.0 Dec 11, 2021
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.

3 participants