Closed dhui closed 3 years ago
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 :)
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.
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
orFileMetadata
? My guess is the former. I think reusingExiftool
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 theexiftool
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/ theexiftool
"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 :)
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
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.
Merging #40 (1101b47) into master (4026654) will decrease coverage by
10.60%
. The diff coverage is50.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%) |
:arrow_down: |
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.
I've updated exiftool
, tests are now passing.
barasher@linux:~/go/src/github.com/barasher/go-exiftool$ exiftool -ver
11.88
Thanks for the fast responses!
Thanks for your involvement !
Other improvement/fixes (each in own commit):
-common_args
option if there are addition init argsexiftool
command to exit onClose()
NewExiftool
in parallel (for me, test runtimes dropped from 3-4s to ~1s)If you'd prefer, I'm happy to break this PR out into multiple smaller PRs.