Skip to content
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

"NoPrintConversion" functional option #24

Merged
merged 4 commits into from
Mar 30, 2021
Merged

"NoPrintConversion" functional option #24

merged 4 commits into from
Mar 30, 2021

Conversation

Kjeldgaard
Copy link
Contributor

New function which adds initial arguments to be parsed to exiftool.

Added a function to parse additional initial arguments to exiftool.

Signed-off-by: Kjeldgaard <Kjeldgaard@users.noreply.github.com>
Added comments and sample to AddInitArgs() function.

Signed-off-by: Kjeldgaard <Kjeldgaard@users.noreply.github.com>
@barasher barasher self-requested a review March 24, 2021 21:03
@barasher
Copy link
Owner

Hi @Kjeldgaard, thanks for the PR !

This feature is bothering me:

  • 👍 on one side, it enabled exiftool tuning to fit the user's need
  • 👎 on the other side, it can completely break go-exiftool if exiftool behaviour changes too much
    For instance, if the provided parameters change the extracted metadata serialization in a way that go-exiftool does not support, the library will behave incorrectly

Exiftool supports so many parameters that it will be really hard to test and filter.

But since I consider that this feature might interest some users, let's do this this way:

  • add some unit tests to your code
  • tell clearly in the godoc comments that this feature is "dangerous" (unsafe) if not well mastered, that extra parameter might break the library and that most of the users should not use this feature
  • add a paragraph Advanced usage in the README.MD and give a real example for this functional option

@barasher barasher added this to the v1.4.0 milestone Mar 24, 2021
@Kjeldgaard
Copy link
Contributor Author

Hi barasher,

Thanks for your comment. I totalky understand your concerns about this change, as it could break go-exiftool functionality. Since it is actually only the "-n" argument I need, I have come to the conclusion that it would probably be better to achieve this similarly to the charset implementation. What do you think?

@barasher
Copy link
Owner

I've taken a look at exiftool's -n parameter : I agree with you, a dedicated "functional option" seems to be the best option.
Deal ! Can you handle it ?

New dedicated function for 'No Print Conversion' mode.

Signed-off-by: Kjeldgaard <Kjeldgaard@users.noreply.github.com>
@Kjeldgaard
Copy link
Contributor Author

I've made a new dedicated function for 'No Print Conversion'. What do you think?

exiftool.go Outdated
}

// Function to use 'No print conversion' mode, see https://exiftool.org/exiftool_pod.html.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation does not fit godoc specifications. The comment should start with the name fonction.
https://blog.golang.org/godoc

@barasher
Copy link
Owner

Just a little godoc fix and that will be perdect :)

Fix NoPrintConversion comment to follow godoc spec.

Signed-off-by: Kjeldgaard <Kjeldgaard@users.noreply.github.com>
@Kjeldgaard
Copy link
Contributor Author

Didn't know that was a thing :) It should be fixed now.

@barasher
Copy link
Owner

Thanks a lot @Kjeldgaard , I'll merge this feature tomorrow and release a new version

@barasher barasher changed the title Function to add initial arguments "NoPrintConversion" functional option Mar 29, 2021
@Kjeldgaard
Copy link
Contributor Author

You're welcome. Glad I could contribute to go-exiftool.

@barasher barasher merged commit 61f2b92 into barasher:master Mar 30, 2021
@Kjeldgaard Kjeldgaard deleted the AddExtraArguments branch March 31, 2021 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants