search

sylikc / pyexiftool

PyExifTool (active PyPI project) - A Python library to communicate with an instance of Phil Harvey's ExifTool command-line application. Runs one process with special -stay_open flag, and pipes data to/from. Much more efficient than running a subprocess for each command!
Other
144 stars 17 forks source link

`copy_tags` Encode parameters + tests for TestTagCopying #19

Closed jangop closed 3 years ago

jangop commented 3 years ago

Currently, copy_tags fails because the parameters are not encoded properly.

Now, I don't necessarily understand all this encoding of parameters, and I don't know why execute cannot take care of it, although this was apparently tried:

https://github.com/sylikc/pyexiftool/blob/1e9b4b34abac6d4294a5958494c7b1b4c1391589/exiftool/exiftool.py#L442

Anyways, set_tags_batch and set_keywords_batch encode their parameters before passing them along, and doing to for copy_tags works in my local monkeypatched version of pyexiftool.

sylikc commented 3 years ago

I'm right there with you @jangop ... I don't quite understand the encoding stuff. I see that the original author had many commits regarding encoding stuff and it seems to be a constant problem. (I commented on a fork that does localized encoding, and that dev said they had problems with UTF-8 on a French localization).

Is there a test case I can test this against so I can do a before and after comparison (and probably write it into a real test case) to make sure it doesn't ever break in the future?

jangop commented 3 years ago

I guess that once you no longer support Python 2.7, we should be able to figure out the entire encoding business more easily.

I actually wrote a test for copy_tags first and then figured out what was wrong. Unfortunately, the test still fails, due to other issues. I'll prepare a simplified version.

sylikc commented 3 years ago

Ok, I see how copy_tags didn't work before. Yeah the .encode('UTF-8') is to turn it into bytes from a string. I forgot when I merged that PR from the old repo, but obviously that code hasn't worked since I merged it.

I'll merge this PR, thanks for adding in a test. I think the v0.5.x will deal with lots of this through parameter checking

The test you wrote passes for me

sylikc commented 3 years ago

@jangop do you need a PyPI release for this PR?

jangop commented 3 years ago

@jangop do you need a PyPI release for this PR?

I was about to say “no” because it is easily monkey patched. However, I could see a number of users installing this module because they want to copy/transplant tags, so maybe go for it. 👍

Thank you for merging.

sylikc commented 3 years ago

released to PyPI. Thanks for the bugfix and tests!