Skip to content

Conversation

@hassec
Copy link
Member

@hassec hassec commented Jan 15, 2021

These were easy enough to add, and I figured they might be useful.

Added a quick test based on the Nikon z6 file (CH0_0174.exv) that is already in the repo to test the version 8 lens data.

@clanmills
Copy link
Collaborator

clanmills commented Jan 15, 2021

Thanks @hassec. A couple of comments:

  1. Do we need print0x0034? There's a macro to match values to string definitions for prettyPrinters. I think expects the value to be an index. However it would be nice to have a variant of that in which the value value is a key to a sparse vector of strings. We might be able to use that to simplify lots of pretty printers. What do you think?

  2. You can put more than one test into a python script. Have a look at test_issue_981.py You can almost certainly add the test from tests/bugfixes/github/test_pr_1444.py to tests/bugfixes/github/test_pr_1437.py. And that makes sense because this PR is really a supplement to Implement Nikon LensData version 8 handler (0.27->master) #1437.

@clanmills clanmills self-requested a review January 15, 2021 14:44
@clanmills clanmills added this to the v0.27.4 milestone Jan 15, 2021
Copy link
Collaborator

@clanmills clanmills left a comment

Choose a reason for hiding this comment

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

Have a look at nikonFlashComp. Its a TagDetails array, which is a sparse array of value/strings. Can you use something similar to eliminate the need for print0x0034?

@hassec
Copy link
Member Author

hassec commented Jan 15, 2021

Thanks for the quick review @clanmills :)

I've moved the test into the one you suggested.

Re print0x0034, I'm not sure what would be best. As you already said, the EXV_PRINT_TAG macro unfortunately won't work for this without modification.
That's why I just went along with the style of the file and created another printXYZ function.
I think for this particular case it makes for a compact PR that can't really break anything existing.
If it was up to me I'd leave a larger refactoring of the overall printing functionality for a new PR that can target master and use C++11.

Edit: had misunderstood EXV_PRINT_TAG

@clanmills
Copy link
Collaborator

You're right. We shouldn't do a big edit on working code.

However, if you can get things to work with a TagDetails array instead of print0x0034() I would much prefer this.

@hassec
Copy link
Member Author

hassec commented Jan 15, 2021

Thanks for the suggestion @clanmills! I updated my comment above, I had actually misunderstood the macro.

Just adapted to code accordingly. It's now even shorter and thus nicer ;)

Copy link
Collaborator

@clanmills clanmills left a comment

Choose a reason for hiding this comment

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

Good Job. Very nice job.

Very clever of you to add the extra -g shutter/i stuff to the existing test. Less is always more in software.

@clanmills
Copy link
Collaborator

When the CI is green, please merge.

@hassec
Copy link
Member Author

hassec commented Jan 15, 2021

@clanmills I don't have the rights to merge

@clanmills clanmills merged commit 57bdd27 into Exiv2:0.27-maintenance Jan 15, 2021
@clanmills
Copy link
Collaborator

clanmills commented Jan 15, 2021

Let me look at the permissions. You're obviously a very good engineer. I'm happy for you to merge.

@clanmills
Copy link
Collaborator

I thought I added you last week. Anyway, you should be in now.

When you want to submit a PR in future, can you create it in the exiv2/exiv2 repos. Much easier work-flow. Other team members can edit/update it. And easier for me to sync/build/review.

@hassec
Copy link
Member Author

hassec commented Jan 15, 2021

Awesome! Thanks a lot, it's much appreciated 👍

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.

2 participants