Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement minimal EXIF and IPTC tag support #40
base: master
Are you sure you want to change the base?
Implement minimal EXIF and IPTC tag support #40
Changes from 1 commit
e393a61
d3698cf
25f5ce0
06f44fd
d3511e1
98a2313
f4f2b42
f8059a7
6bcad51
9e6e742
0a6983f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
What's the rationale for not having a
_load_iptc
method, similar to the_load_exif
method?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.
Good point & implemented.
The only downside is that it now always attempts to load IPTC. We always needed exif for orientation but we could lazy-load IPTC on first use. Right now it will simply load both.
The benefit of the code alignment may outweigh the performance penalty for users that don't use iptc data. I haven't tested the performance penalty yet.
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.
I'm not sure I understand why you can't lazy load IPTC anyway. In
_get_tag_value
, you can load IPTC when needed.Instead of:
you could use:
Then, in
def iptc(self)
, you can have:This means you won't have
_load_iptc
in the same place as_load_exif
, but I think that's acceptable.Let me know if you're interested in making this change. This has been open for a long time so if you're not, we could also just merge it and I can handle it instead.
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.
I'm not a huge fan of overloading
__getattribute__
like this, especially because you're catching anAttributeError
and then looking in the settings map. What if in the future I unintentionally introduce a class variable that's also an exif tag name? I'm not entirely sure where you're using this, but perhaps aget_tag
method can work instead? Happy to discuss this if you'd like.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.
The reason for overloading
__getattribute__
is so that you can access those tags from within the hugo shortcode. That would allow you to expand the shortcode with, for example,{{ .Get "aperture"}}
. I couldn't figure out another way to make that data accessible when processing the shortcode, except for explicitly defining each as a property. I'm definitely open to other ideas.I do understand your objection though and as it isn't quite immediately related to the exif/iptc feature I'm removing this from this branch until we can find out a better solution.
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.
Sorry for the slow response!
This makes sense, Here's one solution I can think of: require a prefix when used in a shortcode:
{{ .Get "exit:ApertureValue" }}
or{{ .Get "iptc:ReleaseTime" }}
. Then you can overload the__getattribute__
method by first checking for theexif
oriptc
prefixes, and retrieving them using_get_tag_value
. If they don't start with either prefix, returngetattr(self, attr)
.