search

drewnoakes / metadata-extractor

Extracts Exif, IPTC, XMP, ICC and other metadata from image, video and audio files
Apache License 2.0
2.55k stars 479 forks source link

Issue 508 review - bplist handling #531

Closed nosnhojbob closed 3 years ago

nosnhojbob commented 3 years ago

Instead of throwing an exception when the bplist array is invalid, I moved the isValid() check ahead. Refactored the AppleRunTime handling into its own class. An error will be inserted into the AppleMakernoteDirectory if the bplist is invalid.

nosnhojbob commented 3 years ago

@drewnoakes - would you mind reviewing this PR?

Thanks - Bob

drewnoakes commented 3 years ago

Apologies for the delay in reviewing this. Thanks, it looks great.

I tweaked the formatting and reinstated the validation in separate commits. I didn't have permission to push to your PR so pushed directly to main. Your commit is now in master (https://github.com/drewnoakes/metadata-extractor/commit/6d2542264c40630dc0e9240068e617db1e66595f). I half expected GitHub to realise this PR was merged, but it hasn't done so. So unfortunately I have to close this. It's basically merged though.

nosnhojbob commented 3 years ago

Thank you! If anything stands out (incorrectly) with how I created this branch and PR, please let me know. Sorry for the troubles with the permissions

drewnoakes commented 3 years ago

Thank you! If anything stands out (incorrectly) with how I created this branch and PR, please let me know. Sorry for the troubles with the permissions

No stress, thank you for the contribution. I don't like closing contribution PRs, especially when the contribution is merged. Your commit is in the history though. If anything I'd say this was a GitHub bug, and that it should have recognised this PR as being merged, but perhaps I'm missing some reason GitHub can't do that.

I believe there is a non-obvious checkbox when you create a PR that controls whether others can push to your branch. It's up to you how you want to set that, but it can allow for extra kinds of collaboration.

paperboyo commented 2 years ago

Your commit is now in master (6d25422)

Hello, @drewnoakes, sorry to bother, but we are potentially hit by this bug. Are you possibly planning a new release, maybe? Thank you!