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

Add inverse mappings for exif tags #6586

Closed
wants to merge 5 commits into from

Conversation

melonmouse
Copy link

@melonmouse melonmouse commented Sep 16, 2022

A nicer solution would be to switch to an enum with the right values, but this is the quick win.

Tested locally, ran through pycodestyle. To me it seems the code is trivial enough to not warrant adding a unit test.

A nicer solution would be to switch to an enum with the right values, but this is the quick win.
@radarhere radarhere changed the title Add inverse mappings for exif tags. Add inverse mappings for exif tags Sep 16, 2022
@radarhere radarhere added the Exif label Sep 16, 2022
:type: dict

The GPS_CODES dictionary maps descriptive string names to 8-bit integer EXIF
gps enumerations. For instance:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
gps enumerations. For instance:
GPS enumerations. For instance:

@radarhere
Copy link
Member

radarhere commented Sep 16, 2022

Could you describe an example use case for this feature? The current dictionary allows for the codes to be translated into something that can be understood by humans. What is your particular scenario where you want to go the other way?

@radarhere
Copy link
Member

A nicer solution would be to switch to an enum with the right values, but this is the quick win.

Could you expand on this?

@melonmouse
Copy link
Author

Suppose a user wants to set an exif tag, I presume they would want to set the data using a human readable tag name.

@melonmouse
Copy link
Author

A nicer solution would be to switch to an enum with the right values, but this is the quick win.

Could you expand on this?

Using a dict to store these values feels hacky in the first place.

from enum import IntEnum


class ExifTags(IntEnum):
    InteropIndex = 0x0001
    ProcessingSoftware = 0x000B
    [...]

Which gives us just the right access:

$ExifTags(0x0001).name
InteropIndex

$ExifTags.InteropIndex.value
1

@melonmouse
Copy link
Author

melonmouse commented Sep 16, 2022

To elaborate a bit more.

Currently

exif_data = Image.Exif()
exif_data[0x123456] = '2020:01:01 00:00:00'
image.save(filename, exif=exif_data.tobytes())

People have to manage how to keep it readable themselves. Their options include adding a comment, rolling their own inverse mapping or accepting less readable code.

After this pull request

exif_data = Image.Exif()
exif_data[TAG_CODES['DateTimeOriginal']] = '2020:01:01 00:00:00'
image.save(filename, exif=exif_data.tobytes())

Here we get the mapping for free, so we don't need to worry about the numerical exif ID (which, to the user, is an implementation detail). The code will crash if there is a typo in the name (which is a good thing) but your favorite IDE may have trouble autocompleting a tag name.

After bigger (breaking?) enum change

exif_data = Image.Exif()
exif_data[ExifTags.DateTimeOriginal] = '2020:01:01 00:00:00'
image.save(filename, exif=exif_data.tobytes())

Here your IDE will autocomplete the tagname. May need to update the code of ExifTags a bit to do this right. Could provide a TAGS dict generated from the Enum for backward compatibility.

@radarhere
Copy link
Member

Here your IDE will autocomplete the tagname. May need to update the code of ExifTags a bit to do this right. Could provide a TAGS dict generated from the Enum for backward compatibility.

I'm in favour of this, but when we previously had this conversation in #5875, I implemented it, it was suggested that the original attributes be deprecated, and now there's #6537 where a user doesn't like that we changed the API. So I might hold off on this for a moment.

@melonmouse
Copy link
Author

Whenever adding anything to an API, it is important to think about a potential future cost of deprecating it again.

Deprecation of the old approach would be costly indeed. It is always a balance between quality of a library and the amount of maintenance for existing users. It is never possible to please everyone, but we can try our best. For a change like the one suggested in this thread, backward compatibility should win. Adding the Enum in a backward compatible way is possible however. I'm sorry about not noticing the backward compatibility break in #5954.

In any case, thank you for your hard work on PIL!

@radarhere radarhere mentioned this pull request Oct 1, 2022
@radarhere
Copy link
Member

I've created PR #6630 to use enums as a possible alternative to this.

@radarhere
Copy link
Member

#6630 has been merged.

@radarhere radarhere closed this Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants