Skip to content

Breaking: Remove unnecessary exception and null checks #63

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Apr 6, 2023
Merged

Breaking: Remove unnecessary exception and null checks #63

merged 10 commits into from
Apr 6, 2023

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

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

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

@qwerty287
Copy link
Author

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