Skip to content

Adding the PrintGroupNames option #67

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
Jun 7, 2023
Merged

Conversation

agorman
Copy link
Contributor

@agorman agorman commented May 4, 2023

I'd like to use the -G option so I'd like to add the PrintGroupNames method.

Thanks!

@agorman
Copy link
Contributor Author

agorman commented May 9, 2023

Bump. Don't want to bother you but hoping you take a look if you have a second.

@barasher
Copy link
Owner

barasher commented May 11, 2023 via email

@agorman
Copy link
Contributor Author

agorman commented May 11, 2023

@barasher thank you for the response. Enjoy your holiday. Talk when you get back.

@agorman
Copy link
Contributor Author

agorman commented May 22, 2023

@barasher I believe you should be back now. If so can you take a look. If not then feel free to ignore this until you return.

Thanks!

@barasher
Copy link
Owner

barasher commented Jun 4, 2023

Hi @agorman !
Once again sorry for the delay.

There's a little thing that bothers me with this implementation.
You've chosen to use the -G option that is a shortcut to -G0. It fits your use-case, but I think that it isn't generic enough : some other people might prefer the -G1 option for instance.

What about adding another parameter that specifies which "number" to use ?

Sincerely,
Arnaud

@agorman
Copy link
Contributor Author

agorman commented Jun 4, 2023

Great point. Because the -G option allows for multiple group number (-G[*NUM*][:*NUM*...]) I can change the PrintGroupNames function to take a string. That way a 0 could be passed or multiple group numbers like 3:1.

What do you think?

@agorman
Copy link
Contributor Author

agorman commented Jun 4, 2023

Alternatively I could change PrintGroupNames to take ...int. That way if nothing was passed I could assume 0. Otherwise I could create the group names string based on the passed ints.

@barasher
Copy link
Owner

barasher commented Jun 4, 2023

Great point. Because the -G option allows for multiple group number (-G[*NUM*][:*NUM*...]) I can change the PrintGroupNames function to take a string. That way a 0 could be passed or multiple group numbers like 3:1.

What do you think?

I think that's the best option ! A string parameter would be great.

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.26 🎉

Comparison is base (caa7545) 80.54% compared to head (c2a9c7d) 80.80%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #67      +/-   ##
==========================================
+ Coverage   80.54%   80.80%   +0.26%     
==========================================
  Files           2        2              
  Lines         293      297       +4     
==========================================
+ Hits          236      240       +4     
  Misses         40       40              
  Partials       17       17              
Impacted Files Coverage Δ
exiftool.go 72.85% <100.00%> (+0.52%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@barasher
Copy link
Owner

barasher commented Jun 5, 2023

It seems great !

@barasher barasher added the enhancement New feature or request label Jun 5, 2023
@barasher barasher added this to the v1.10.0 milestone Jun 5, 2023
@barasher barasher self-requested a review June 5, 2023 06:28
@barasher
Copy link
Owner

barasher commented Jun 7, 2023

When running unit-tests, I have an error:

--- FAIL: TestPrintGroupNames (0.65s)
    exiftool_test.go:465: 
        	Error Trace:	exiftool_test.go:465
        	Error:      	Expected nil, but got: &errors.errorString{s:"key not found"}
        	Test:       	TestPrintGroupNames
    exiftool_test.go:466: 
        	Error Trace:	exiftool_test.go:466
        	Error:      	Not equal: 
        	            	expected: "64"
        	            	actual  : ""
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-64
        	            	+
        	Test:       	TestPrintGroupNames

I tried:

barasher@linux:~/go/src/github.com/barasher/go-exiftool$ exiftool -j -G0:1:2:3:4:5:6:7 ./testdata/20190404_131804.jpg | grep Width
  "EXIF:ExifIFD:Image:Main:JPEG-APP1-IFD0-ExifIFD:int32u:ExifImageWidth": 4032,
  "EXIF:IFD1:Image:Main:Copy1:JPEG-APP1-IFD1:int32u:ImageWidth": 504,
  "File:Image:Main:JPEG-SOF0:ImageWidth": 64,

My exiftool version:

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

What's your version ? Mine is outdated, I'll update it and try again.

@barasher
Copy link
Owner

barasher commented Jun 7, 2023

I've updated exiftool and all tests are passing

@barasher barasher merged commit 3d2245a into barasher:master Jun 7, 2023
@barasher
Copy link
Owner

barasher commented Jun 7, 2023

Once more, thanks for the PR !
Released in v1.10.0

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants