Skip to content

Breaking: Remove unnecessary exception and null checks#63

Merged
qwerty287 merged 10 commits intomasterfrom
null-checks
Apr 6, 2023
Merged

Breaking: Remove unnecessary exception and null checks#63
qwerty287 merged 10 commits intomasterfrom
null-checks

Conversation

@qwerty287
Copy link

@qwerty287 qwerty287 commented Apr 6, 2023 •
edited
Loading

Breaking:

  • removes Reader::getAdapter()
  • removes Reader::getExifFromFile()
  • removes NoAdapterException

@qwerty287 qwerty287 requested a review from a team April 6, 2023 07:20
@codecov
Copy link

codecov bot commented Apr 6, 2023 •
edited
Loading

Codecov Report

Merging #63 (bd83c45) into master (8460af5) will decrease coverage by 0.20%.
The diff coverage is 98.52%.

❗ Current head bd83c45 differs from pull request most recent head ab0160e. Consider uploading reports for the commit ab0160e to get more accurate results

Additional details and impacted files

@ildyria
Copy link
Member

ildyria commented Apr 6, 2023

It is breaking, so this will require bump to version 1.0.0

@qwerty287
Copy link
Author

qwerty287 commented Apr 6, 2023

Are there other changes that might be breaking? Then we should get them in here too

@ildyria
Copy link
Member

ildyria commented Apr 6, 2023 •
edited
Loading

Not that I can think of.

We should return a proper exception instead of a RuntimeException somewhere in the code.

@qwerty287
Copy link
Author

qwerty287 commented Apr 6, 2023

Yes, give me some time to fix it...

@qwerty287
Copy link
Author

qwerty287 commented Apr 6, 2023

I'm not sure what I should use for https://github.com/LycheeOrg/php-exif/blob/master/lib/PHPExif/Adapter/Exiftool.php#L187 and for https://github.com/LycheeOrg/php-exif/blob/master/lib/PHPExif/Adapter/Exiftool.php#L146 ... Any ideas from your side?

ildyria
ildyria reviewed Apr 6, 2023
View reviewed changes
@ildyria
Copy link
Member

ildyria commented Apr 6, 2023

I'm not sure what I should use for master/lib/PHPExif/Adapter/Exiftool.php#L187 and for master/lib/PHPExif/Adapter/Exiftool.php#L146 ... Any ideas from your side?

I would just use an Umbrella exception like PhpExifReaderException and documents with @throws.

qwerty287 and others added 4 commits April 6, 2023 11:33
@qwerty287 @ildyria
Update lib/PHPExif/Reader/Reader.php
29b00fe 
Co-authored-by: Benoît Viguier <ildyria@users.noreply.github.com>
@qwerty287
New exceptions
e3bb5a1
@qwerty287
fix constructor
84c7873
@qwerty287
fix format
bd83c45
@qwerty287 qwerty287 requested a review from ildyria April 6, 2023 09:47
ildyria
ildyria approved these changes Apr 6, 2023
View reviewed changes
Copy link
Member

@ildyria ildyria left a comment

Choose a reason for hiding this comment

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

LGTM.

@ildyria
Copy link
Member

ildyria commented Apr 6, 2023

Maybe add two codecov ignore in the try catch of https://app.codecov.io/gh/LycheeOrg/php-exif/pull/63/blob/lib/PHPExif/Adapter/Exiftool.php ?

@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@qwerty287 qwerty287 merged commit bfc651d into master Apr 6, 2023
@qwerty287 qwerty287 deleted the null-checks branch April 6, 2023 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ildyria ildyria ildyria approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@qwerty287 @ildyria