-
Notifications
You must be signed in to change notification settings - Fork 24
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.
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
Support multiple values with Array #34
base: master
Are you sure you want to change the base?
Conversation
@@ -5,6 +5,7 @@ | |||
#include <unistd.h> | |||
#include <string> | |||
#include <map> | |||
#include <list> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why you went with a list rather than say a vector?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For most cases this list will contain single element. And all manipulations, such as conversion to v8::Array, happen to iterate through all values. Given that we don't need random access to elements and want the quick addition, "list" should be a good choice here.
Looking through this, I really like the idea, but I'm wondering if by returning both arrays and strings we'll be breaking backwards compatibility. If people are assuming they'll get a string back and suddenly get an array that's going to cause problems. In that case I'd almost rather just always get an array back rather than having to check the type and then loop or not. Thoughts? /cc @dberesford |
Most of the values are single, so returning array for each value is not very useful. Moreover, if we keep strings for single-value tags, the majority of existing code will still work. |
Looks good to me, nice job! @drewish I'm happy to merge this, how about you? |
+1 Was just about to submit a bug report. Currently returned This patch should help. |
@dragn Any chance this could be rebased so it can be merged? |
If we do merge this we need to bump the major version since it's not backwards compatible. Also should have an example in the readme since it's a little odd to have to check if it's a string or array. I still think that's going to be a confusing interface. |
@drewish I think if we'd change it to return (and expect) tags as a single string with comma-separated values, we'd have the same functionality and backward compatibility, except for the fact that you can't use comma in tags... |
How about an option to choose how multiple values are handled? Something like |
Add support to set and get tags with multiple values.
getImageTags
will return a string for single value or an array of strings for multiple values.setImageTags
will add multiple records with the same key when array is provided.Solves #23
I'm just starting to use that in my own project, so any issues may pop up later.
Using localized text in tags would certainly impose some difficulties... (Usage of Exiv2::AsciiValue, for example...).
I don't really like that part with a lot of copy-paste stuff for ExifData/IptcData/XmpData. But, given that they don't derive from the same class, I have no idea what to do with it (except using macros, which would not do much better).