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

Fix source files modified on import #252 #272

Merged
merged 9 commits into from Feb 4, 2019

Conversation

Projects
None yet
5 participants
@mattca
Copy link
Contributor

mattca commented Apr 5, 2018

No description provided.

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Apr 5, 2018

CLA assistant check
All committers have signed the CLA.

@mattca

This comment has been minimized.

Copy link
Contributor Author

mattca commented Apr 5, 2018

Tests failing, will have a look

@mattca

This comment has been minimized.

Copy link
Contributor Author

mattca commented Apr 6, 2018

Updated idea to tweak checksum/hashing logic

@jmathai

This comment has been minimized.

Copy link
Owner

jmathai commented Apr 6, 2018

I'll have a look at why these tests are failing as well.

@mattca

This comment has been minimized.

Copy link
Contributor Author

mattca commented Apr 8, 2018

I have fixed some more tests locally but this PR is still a WIP. Realised I think I can tidy up to make use of exiftools -o flag, will investigate...

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 8, 2018

Coverage Status

Coverage increased (+0.003%) to 93.127% when pulling 4a15b0f on mattca:source_files_are_modified_on_import_252 into 6bde290 on jmathai:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 8, 2018

Coverage Status

Coverage decreased (-1.7%) to 90.074% when pulling 6f8c750 on mattca:source_files_are_modified_on_import_252 into 086c26c on jmathai:master.

@mattca mattca force-pushed the mattca:source_files_are_modified_on_import_252 branch from 4a15b0f to 7937832 Apr 10, 2018

@mattca

This comment has been minimized.

Copy link
Contributor Author

mattca commented Apr 12, 2018

Had a look at making use of exiftools -o flag so we don't rely on the *_original file created. However, pyexiftool seems to break with this option. So leaving as it is. @jmathai can you review the PR, do you think we're OK to merge?

Thanks!

1 similar comment
@mattca

This comment has been minimized.

Copy link
Contributor Author

mattca commented Apr 12, 2018

Had a look at making use of exiftools -o flag so we don't rely on the *_original file created. However, pyexiftool seems to break with this option. So leaving as it is. @jmathai can you review the PR, do you think we're OK to merge?

Thanks!

@jmathai
Copy link
Owner

jmathai left a comment

This is great. Thanks for the PR and especially for updating the tests. I left a few minor comments.

Show resolved Hide resolved elodie/filesystem.py Outdated
Show resolved Hide resolved elodie/filesystem.py Outdated
Show resolved Hide resolved elodie/filesystem.py Outdated
Show resolved Hide resolved elodie/filesystem.py Outdated
Show resolved Hide resolved elodie/filesystem.py Outdated
Show resolved Hide resolved elodie/filesystem.py Outdated
Show resolved Hide resolved elodie/filesystem.py
Show resolved Hide resolved elodie/filesystem.py
Show resolved Hide resolved elodie/tests/filesystem_test.py Outdated
@fredmorin

This comment has been minimized.

Copy link

fredmorin commented Jan 2, 2019

Any chance this gets merged in the master branch? I'd like to use elodie, but do not want my original files to be modified. I attempted to import a folder and also noticed the issue.

@jmathai

This comment has been minimized.

Copy link
Owner

jmathai commented Jan 22, 2019

This PR is next on my list to get merged. I've lost most of the context around it but will try and ramp up again.

@mattca

This comment has been minimized.

Copy link
Contributor Author

mattca commented Jan 23, 2019

Hi, apologies, I know it's been a while but I'd like to pick this back up. Will have another look sometime this week. Have also lost track but I think I just need to look into restoring the timestamp. Will give you an update on how it goes.

mattca added some commits Jan 24, 2019

Merge pull request #1 from jmathai/master
PR to merge master from upstream

@mattca mattca force-pushed the mattca:source_files_are_modified_on_import_252 branch from 7bc803a to 7b04af1 Jan 24, 2019

mattca added some commits Jan 24, 2019

@mattca

This comment has been minimized.

Copy link
Contributor Author

mattca commented Jan 24, 2019

OK, @jmathai can you re-review? I rebased on most recent code and made some final tweaks. Tests now passing and did a small test locally and the checksums etc seem fine. Had to do a bit of a hack around the text file, am having to copy that to an "_original" file as well in the same way exiftool does. Seems to work.. Haven't tried it with lots of files yet, will do a more real world test just to be confident when I get a second.

@jmathai

This comment has been minimized.

Copy link
Owner

jmathai commented Jan 25, 2019

Thanks! I'm traveling the next few days but will have a look early next week.

@jmathai

This comment has been minimized.

Copy link
Owner

jmathai commented Feb 3, 2019

Hey @mattca -- Looks great. I've submitted a PR against your branch and can merge into here once you merge there.

Merge pull request #2 from jmathai/mattca-source_files_are_modified_o…
…n_import_252

Add new test, update comments and delete photo.cr2
@jmathai

jmathai approved these changes Feb 4, 2019

@jmathai jmathai merged commit 2e2c103 into jmathai:master Feb 4, 2019

4 checks passed

Scrutinizer Analysis: 44 new issues, 2 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-1.7%) to 90.074%
Details
license/cla Contributor License Agreement is signed.
Details

@jmathai jmathai removed the do not merge label Feb 4, 2019

@jmathai

This comment has been minimized.

Copy link
Owner

jmathai commented Feb 4, 2019

Thanks @mattca!

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