Parsing canon maker notes #99
Conversation
|
@v1r0x Thanks for the pull request. Before a possible merge, you need to get the tests to not fail. https://travis-ci.org/pel/pel/jobs/281884652 Also, it would be awesome if you could add a PHPUnit test for the functionality you added, so we know it will not break later on e.g. the Nikon maker notes. |
|
@lsolesen I have a look at travis soon. Want to parse the sub-notes before a merge anyway ;) |
|
I think I'm finished. Most of the known tags are parsed and translated to english. I didn't add german translations, because it looks like they aren't available for other tags too. I'd translate them in another PR if desired. Is this ok or anything I should fix for a merge @lsolesen ? |
src/PelCanonMakerNotes.php
Outdated
| $offset += 2; | ||
| $elemSize = PelFormat::getSize(PelFormat::SSHORT); | ||
| if ($size / $components !== $elemSize) { | ||
| // TODO throw |
There was a problem hiding this comment.
Should an exception be thrown in this case (and in similar other cases?)
There was a problem hiding this comment.
You're right. Embarrassing I missed the TODOs...
One problem is, that I don't know if it is correct to throw an exception at this point. Since there are unknown tags which could be of another format. Thus it would throw an PelMalformedMakerNotesException or something else, just because there is an unknown tag.
What's your opinion? I'm ok with both solutions (1. remove the checks and 2. keep them and adjust if there is a case in the future where the exception is thrown although it is a valid maker notes section).
There was a problem hiding this comment.
I would throw an exception so we would get bug reports if there is something wrong. Code could be improved. Otherwise strange results could be seen. I haven't worked with those parsers yet, so I don't really knw about the impact.
There was a problem hiding this comment.
Ok, will add a exception the next days. Do you refer to a specific part for improvement?
Edit: Or do you refer to improvements if someone reports a bug due to a thrown PelMalformedMakerNotesException?
There was a problem hiding this comment.
My idea is that we might see exception reports when users report PelMalformedMakerNotesException .
There was a problem hiding this comment.
Please have a look at the new scruntizer tests which show 43 new issues and a decrease in code coverage.
There was a problem hiding this comment.
Most of the issues are not caused by this PR. Code Coverage is difficult. E.g. PelEntrySShort got from 100% code coverage to ~2%, because I had to add the getText method. It should be possible to increase it a bit, but I added more than 1000 Lines and almost 700 are for different lenses. Thus I'm not sure how much it could be increased and what would be a good goal.
And since scrutinizer was added today I'm not sure if the results are representative in any way. But I'll have a look at the issues/code coverage caused by this PR.
|
@weberhofer I added some tests for the eos 650d and some other small bugs and the TODOs as well. Is this ok? |
|
In general it's good, what I don't like is scruntinizer's "43 new issues, 27 updated code elements". Those are mainly coming from the long switch constructions which we should avoid in the future. But maybe this should be done separately by refactoring e.g. PelEntryShort. |
|
Yeah, I understand that the issues in scrutinizer should be fixed, but they are caused (as you said) by the switch statements, etc. |
|
Thanks! If you have resources: Improvements are highly welcome! |
|
Thanks for the merge! If you have anything were I can help, just @ mention me. |
This PR is my first attempt of parsing canon's maker notes. The results look good so far. Beside FocalLength, which is overwritten by the maker notes and has an other format.
Only first level elements are taken into account. E.g. tag
0x0001(Exif.Canon.CameraSettings) is not parsed yet.Would be great if anyone else could test and have a look at my code if it is ok :)
As soon as this one gets (hopefully) merged, I'd have a look at nikon's maker notes.