Skip to content
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

Handling of 0..unbounded cardinality fields such as iptc Keywords #203

Open
krisb opened this issue Aug 16, 2022 · 2 comments
Open

Handling of 0..unbounded cardinality fields such as iptc Keywords #203

krisb opened this issue Aug 16, 2022 · 2 comments

Comments

@krisb
Copy link

krisb commented Aug 16, 2022

Description

In issue #105 and pr #106, Keywords type was changed from NumberArrayTag to NumberArrayTag[]. However, I note that it is possible to get a single value or an array value back in the API, so this change really aligned with the most likely use of the field rather than being strictly correct.

I think there are two approaches to address this:
a) Normalise fields that can be singular or array values into array values
b) Change the types to correctly represent what the possible values may be, e.g. Type | Type[]

In either case it would be a breaking change, however, my preference would be to normalise the fields to always be array values as this is the most common case and the simplest to code against (I do this in my code base as a defensive measure against the variation).

Looking at https://iptc.org/std/photometadata/specification/IPTC-PhotoMetadata#keywords, I see that there quite a few other fields with cardinality of 0..unbounded, which makes me wonder if this is a more widespread concern in terms of the suggested approach. I would feel that 0..1 should be modelled as a singular optional field and 0..unbounded should be modelled an array with 0 or more entries.

Has this been discussed previously?

After a discussion and an agreement on approach, I'd be happy to work on it and submit a merge request.

Additional details

How to reproduce

  1. Try an image with a single Keyword
  2. Try an image with multiple Keywords

What happened:

In the first case a single object is returned and in the second case an array of objects is returned

What I expected would happen:

Either normalise to always return an array of objects, or change the types to show it could be an array or single value, e.g. NumberArrayTag[] | NumberArrayTag

@mattiasw
Copy link
Owner

Hi! Just want to say I've read this and will have a look as soon as I can, a lot of stuff happening right now. There was probably a reason why it's a single value if there is only one tag, I just have to remember what it was. 😅 I'll get back to you.

@mattiasw
Copy link
Owner

I agree, normalizing is probably the best route here. I vaguely recall some reasoning but can't really formulate it now. Plus, having it always return an array will make it easier for people to create a correct implementation in case they happen to only test with images with a single value.

If you'd like to make the change it's going to be in src/iptc-tags.js. Search for repeatable. That's how to know if the tag can have multiple values. The if statement has to be changed, and the else block. I think that may be it. The tests probably don't have to be updated which also is a sign that the single-value case is not important. After that the typings need to be changed for all repeatable IPTC tags. You can see which ones those are by looking for repeatable in the file src/iptc-tag-names.js.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants