Fix langAltValue::read() parsing #1482
Conversation
+ Fix segmentation faults in langAlt parse
+ Fix mismatched quotation marks and incorrect values
+ Add Python testing
+ Some tests commented out as quotation marks are filtered, preventing them
from running.
Closes Exiv2#1481.
|
Thanks for doing this work @postscript-dev This will be reviewed next week. I hope it will ship in v0.27.4 RC1 which is on track to ship as scheduled for 2021-03-31. |
|
|
||
| url = "https://github.com/Exiv2/exiv2/issues/1481" | ||
|
|
||
| # Python unittest is filtering out empty pairs of "" and flags up an error if |
There was a problem hiding this comment.
Would it be possible to test the method LangAltValue::read() in unit tests instead of the python tests? Maybe there you have more control to use different combinations of quotes.
There was a problem hiding this comment.
I will look into this today.
There was a problem hiding this comment.
Your suggestion was helpful. I have added code to the unitTests and removed the Python version.
There was a problem hiding this comment.
This is good work. Thank You for undertaking this task and adding to the test suite and to the kerErrorCodes.
I'm willing to approve this as it stands (when you fix the spelling of ALPLHA).
Several discussion points:
- I agree with @piponazo. We usually test this kind of code in then unitTests.
- Should anything be added to the man page man/man1/exiv2.1 about this?
- Why is there a lot of stuff commented off in the python code? Should there be a var bFull = False # True in the script prolog to drive in different modes. Why not execute every test?
- Is there a reason for the ALPLHA code instead of std::is alpha?
- Can any python strings which include " be coded as 'I am a dog " ear'. All those dog ears (my name for a quotation mark character) are tough on the eye.
This is Good Work, so please don't take my comments as criticism.
Removed previous Python tests.
|
I am working throught the comments that have been provided and will post when I have finished. |
Removed unneeded comments and empty space.
There was a problem hiding this comment.
I will look at this carefully after dinner. At a glance it looks good. Thanks.
I would like to acknowledge you in the release notes. First name is sufficient: #1018 (comment)
I have made the change.
You are right. As I wasn't changing the format, I didn't think of updating the manual page. I've made the change.
I put the tests in that I couldn't get to work in case someone else had a suggestion. Switching them on/off with a flag is a better idea.
Originally I thought that as
I didn't see your message until later last night. By that time, I had already moved over to unitTest as Luis suggested. I didn't try the single quotes but will try to use them in the future. |
My first name is Peter. |
There was a problem hiding this comment.
@postscript-dev Peter: This is very good work. I see you've thrown away the python code and add the unit test code. Sometimes doing more is the right thing to do.
Yes, you're right about the order and locale. What a can of worms. If anybody ever reports an issue with that, it's likely that they come from a non-ascii locale and will be familiar with such issues and their remedy.
So, I'm going to approve this. When the CI is green, I'll merge. I have already updated the release notes to acknowledge your contribution.
The changes are in response to #1481
Closes #1481.