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

Populate title & description from IPTC image data #297

Merged
merged 2 commits into from Feb 15, 2018

Conversation

2 participants
@edwinsteele
Contributor

edwinsteele commented Feb 13, 2018

The IPTC spec defines title and description fields so use those
to populate these fields on the Image unless they've been set by
some other means, thus preserving existing behaviour.

This provides a pattern that could be used with #249

Populate title & description from IPTC image data
The IPTC spec defines title and description fields so use those
to populate these fields on the Image unless they've been set by
some other means.
@codecov

This comment has been minimized.

codecov bot commented Feb 13, 2018

Codecov Report

Merging #297 into master will increase coverage by 0.17%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #297      +/-   ##
==========================================
+ Coverage   87.48%   87.65%   +0.17%     
==========================================
  Files          19       19              
  Lines        1358     1377      +19     
==========================================
+ Hits         1188     1207      +19     
  Misses        170      170
Impacted Files Coverage Δ
sigal/image.py 94.55% <100%> (+0.28%) ⬆️
sigal/gallery.py 88.2% <100%> (+0.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9004843...45cf226. Read the comment docs.

@saimn

A small comment, otherwise it looks great, thanks!

if iptc_data and not self.description:
# 2:120 is the IPTC description property
if (2, 120) in iptc_data:
self.description = iptc_data[(2, 120)].decode('utf-8')

This comment has been minimized.

@saimn

saimn Feb 14, 2018

Owner

I think you could put the details of how to get title and description in get_iptc_data, it would make the function easier to test and more self-contained, and avoid to do this directly in Image which does not have to know how IPTC work.

This comment has been minimized.

@edwinsteele

edwinsteele Feb 15, 2018

Contributor

Good idea.

@saimn

This comment has been minimized.

Owner

saimn commented Feb 15, 2018

@edwinsteele - What is the license of the images ? If you are the author of these images, do you agree do share them with sigal's license ?

@edwinsteele

This comment has been minimized.

Contributor

edwinsteele commented Feb 15, 2018

@saimn

This comment has been minimized.

Owner

saimn commented Feb 15, 2018

Great, merging then, thanks!

@saimn saimn merged commit c2cdb22 into saimn:master Feb 15, 2018

3 checks passed

codecov/patch 100% of diff hit (target 87.48%)
Details
codecov/project 87.65% (+0.17%) compared to 9004843
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@saimn saimn added this to the 1.4 milestone Feb 17, 2018

@edwinsteele edwinsteele deleted the edwinsteele:feature/get-title-desc-from-iptc branch Oct 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment