Support writing metadata to files + other improvements/fixes #40

dhui · 2021-07-14T05:34:04Z

Other improvement/fixes (each in own commit):

only set -common_args option if there are addition init args
fix typo
wait for exiftool command to exit on Close()
Run tests that call NewExiftool in parallel (for me, test runtimes dropped from 3-4s to ~1s)
Run CI on push and pull requests

If you'd prefer, I'm happy to break this PR out into multiple smaller PRs.

barasher · 2021-07-14T20:46:39Z

Hi @dhui ! Thanks a lot for your contribution. That's a great job !

Could you please split this PR in 2 differents PR, one for your improvements concerning metadata extraction, and the other one concerning the new feature that you're adding, the metadata writing capability.

About the last one, I'm wondering if it would not be more logical to have another "struct" handling the metadata writing...
Since there are no init options that are common between the two modes (read / write), I think it should be clearer to have a struct handling metadata read operations and another one dedicated to the write operations (and, one day, a third one dedicated to binary extraction - issue #34 ). But we'll discuss about it on the dedicated PR :)

dhui · 2021-07-14T23:54:30Z

Could you please split this PR in 2 differents PR, one for your improvements concerning metadata extraction, and the other one concerning the new feature that you're adding, the metadata writing capability.

Here's the other PR w/ various improvements. I'll leave this one open until we've decided on the best approach for handling writing metadata

About the last one, I'm wondering if it would not be more logical to have another "struct" handling the metadata writing...
Since there are no init options that are common between the two modes (read / write), I think it should be clearer to have a struct handling metadata read operations and another one dedicated to the write operations (and, one day, a third one dedicated to binary extraction - issue #34 ). But we'll discuss about it on the dedicated PR :)

Are you referring to struct different from Exiftool or FileMetadata? My guess is the former.
I think reusing Exiftool makes sense for writing metadata. Even though there aren't overlapping options between reading and writing metadata, it'd be nice to be able to use one invocation of the exiftool command (e.g. one sub-process) for both reading and writing. I prefer to provide users more flexibility and and power at the risk of making more mistakes.
There's also shared logic around interacting w/ the exiftool "protocol" but that could be factored out into it's own non-exported struct.
One use case to consider is updating existing file metadata. e.g. changing the timezone of a timestamp or title-casing a title
It's more cumbersome to need to use 2 structs (one for reading the metadata and one for writing the changed metadata) rather than one single struct.

exiftool.go

exiftool_test.go

exiftool.go

exiftool_test.go

exiftool.go

barasher · 2021-07-17T21:27:29Z

Could you please split this PR in 2 differents PR, one for your improvements concerning metadata extraction, and the other one concerning the new feature that you're adding, the metadata writing capability.

Here's the other PR w/ various improvements. I'll leave this one open until we've decided on the best approach for handling writing metadata

About the last one, I'm wondering if it would not be more logical to have another "struct" handling the metadata writing...
Since there are no init options that are common between the two modes (read / write), I think it should be clearer to have a struct handling metadata read operations and another one dedicated to the write operations (and, one day, a third one dedicated to binary extraction - issue #34 ). But we'll discuss about it on the dedicated PR :)

Are you referring to struct different from Exiftool or FileMetadata? My guess is the former.
I think reusing Exiftool makes sense for writing metadata. Even though there aren't overlapping options between reading and writing metadata, it'd be nice to be able to use one invocation of the exiftool command (e.g. one sub-process) for both reading and writing. I prefer to provide users more flexibility and and power at the risk of making more mistakes.
There's also shared logic around interacting w/ the exiftool "protocol" but that could be factored out into it's own non-exported struct.
One use case to consider is updating existing file metadata. e.g. changing the timezone of a timestamp or title-casing a title
It's more cumbersome to need to use 2 structs (one for reading the metadata and one for writing the changed metadata) rather than one single struct.

Ok, you've convinced me :) Let's keep a single "tool" / struct :)

Addresses: barasher#40 (comment)

Use strings.HasSuffix instead of implementing it ourselves Addresses: barasher#40 (comment)

Addresses: barasher#40 (comment)

…adata Addresses: barasher#40 (comment)

dhui · 2021-07-21T21:19:15Z

Looks like tests are failing since they're running with Go 1.13. Any objections running tests with Go 1.15? I'd use testing.T.TempDir()

Error: ./exiftool_test.go:662:17: undefined: os.MkdirTemp
Error: ./exiftool_test.go:676:9: undefined: filepath.WalkDir
Error: ./exiftool_test.go:676:51: undefined: os.DirEntry

barasher

Tests don't pass on my computer:

barasher@linux:~/go/src/github.com/barasher/go-exiftool$ git log -1
commit 87c8ba7226e5dd9ba92ecccbd1673921ccf54b48 (HEAD -> dhui-write)
Author: Dale Hui <[email protected]>
Date:   Sun Jul 18 09:13:20 2021 -0600

    Invert default behavior of keeping the original file when writing metadata
    Addresses: https://github.com/barasher/go-exiftool/pull/40#discussion_r670768486

barasher@linux:~/go/src/github.com/barasher/go-exiftool$ go test ./...
--- FAIL: TestWriteMetadataClearBeforeWriting (0.29s)
    exiftool_test.go:591: 
        	Error Trace:	exiftool_test.go:591
        	            				exiftool_test.go:672
        	            				exiftool_test.go:562
        	Error:      	Not equal: 
        	            	expected: float64(1.5)
        	            	actual  : <nil>(<nil>)
        	Test:       	TestWriteMetadataClearBeforeWriting
--- FAIL: TestWriteMetadata (0.44s)
    exiftool_test.go:470: 
        	Error Trace:	exiftool_test.go:470
        	            				exiftool_test.go:672
        	            				exiftool_test.go:410
        	Error:      	Not equal: 
        	            	expected: float64(1.5)
        	            	actual  : <nil>(<nil>)
        	Test:       	TestWriteMetadata
        	Messages:   	Field CameraImagingModelPixelAspectRatio differs
FAIL
FAIL	github.com/barasher/go-exiftool	1.710s
FAIL

barasher@linux:~/go/src/github.com/barasher/go-exiftool$ exiftool -ver
10.80
barasher@linux:~/go/src/github.com/barasher/go-exiftool$ go version
go version go1.16.2 linux/amd64

exiftool_test.go

barasher · 2021-07-22T19:53:07Z

Looks like tests are failing since they're running with Go 1.13. Any objections running tests with Go 1.15? I'd use testing.T.TempDir()
Error: ./exiftool_test.go:662:17: undefined: os.MkdirTemp
Error: ./exiftool_test.go:676:9: undefined: filepath.WalkDir
Error: ./exiftool_test.go:676:51: undefined: os.DirEntry

Considering golang's backward compatibility policy, that's OK for me.

codecov-commenter · 2021-07-23T05:27:17Z

Codecov Report

Merging #40 (1101b47) into master (4026654) will decrease coverage by 10.60%.
The diff coverage is 50.00%.

@@             Coverage Diff             @@
##           master      #40       +/-   ##
===========================================
- Coverage   89.44%   78.83%   -10.61%     
===========================================
  Files           2        2               
  Lines         180      241       +61     
===========================================
+ Hits          161      190       +29     
- Misses         13       35       +22     
- Partials        6       16       +10

Impacted Files	Coverage Δ
exiftool.go	`70.85% <50.00%> (-12.48%)`	⬇️

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 4026654...1101b47. Read the comment docs.

barasher · 2021-07-23T20:19:08Z

I've updated exiftool, tests are now passing.

barasher@linux:~/go/src/github.com/barasher/go-exiftool$ exiftool -ver
11.88

dhui · 2021-07-23T20:48:36Z

Thanks for the fast responses!

barasher · 2021-07-23T21:12:36Z

Thanks for your involvement !

barasher requested changes Jul 15, 2021

View reviewed changes

exiftool.go Outdated Show resolved Hide resolved

exiftool_test.go Outdated Show resolved Hide resolved

exiftool.go Outdated Show resolved Hide resolved

exiftool_test.go Outdated Show resolved Hide resolved

barasher requested changes Jul 15, 2021

View reviewed changes

exiftool.go Outdated Show resolved Hide resolved

barasher added this to the v1.7.0 milestone Jul 21, 2021

dhui added 8 commits July 21, 2021 14:11

Support writing file metadata

47e6978

Add option to overwrite original file

fc6d55b

Implement copy ourselves

d1e5f3e

Addresses: barasher#40 (comment)

Simplify handleWriteMetadataResponse()

0fbd89d

Use strings.HasSuffix instead of implementing it ourselves Addresses: barasher#40 (comment)

Add more test cases to TestWriteMetadata

d823a27

Addresses: barasher#40 (comment)

Handle fmt.Fprintln() errors

33a2d6d

Support clearing all fields and removing specific fields

5e67a00

Addresses: barasher#40 (comment)

Invert default behavior of keeping the original file when writing met…

87c8ba7

…adata Addresses: barasher#40 (comment)

dhui force-pushed the write branch from a74020a to 87c8ba7 Compare July 21, 2021 21:14

dhui requested a review from barasher July 21, 2021 21:15

barasher requested changes Jul 22, 2021

View reviewed changes

exiftool_test.go Show resolved Hide resolved

dhui added 2 commits July 22, 2021 22:19

Run CI using Go 1.15.x

72bafc1

Make compatible with Go 1.15.x

1101b47

barasher mentioned this pull request Jul 23, 2021

Metadata writing : test array fields #44

Closed

barasher approved these changes Jul 23, 2021

View reviewed changes

barasher added the Write label Jul 23, 2021

barasher merged commit 4141abd into barasher:master Jul 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support writing metadata to files + other improvements/fixes #40

Support writing metadata to files + other improvements/fixes #40

dhui commented Jul 14, 2021 •

edited

Loading

barasher commented Jul 14, 2021

dhui commented Jul 14, 2021 •

edited

Loading

barasher commented Jul 17, 2021

dhui commented Jul 21, 2021

barasher left a comment

barasher commented Jul 22, 2021

codecov-commenter commented Jul 23, 2021

barasher commented Jul 23, 2021 •

edited

Loading

dhui commented Jul 23, 2021

barasher commented Jul 23, 2021

Support writing metadata to files + other improvements/fixes #40

Support writing metadata to files + other improvements/fixes #40

Conversation

dhui commented Jul 14, 2021 • edited Loading

barasher commented Jul 14, 2021

dhui commented Jul 14, 2021 • edited Loading

barasher commented Jul 17, 2021

dhui commented Jul 21, 2021

barasher left a comment

Choose a reason for hiding this comment

barasher commented Jul 22, 2021

codecov-commenter commented Jul 23, 2021

Codecov Report

barasher commented Jul 23, 2021 • edited Loading

dhui commented Jul 23, 2021

barasher commented Jul 23, 2021

dhui commented Jul 14, 2021 •

edited

Loading

dhui commented Jul 14, 2021 •

edited

Loading

barasher commented Jul 23, 2021 •

edited

Loading