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

[COMPRESS-429] Provide information about source of name / comment field values (e.g. Unicode) #56

Closed
wants to merge 2 commits into from

Conversation

dalbani
Copy link
Contributor

@dalbani dalbani commented Nov 20, 2017

@coveralls
Copy link

coveralls commented Nov 20, 2017

Coverage Status

Coverage decreased (-0.01%) to 85.869% when pulling 36d504d on dalbani:COMPRESS-429 into 10ff1c3 on apache:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 85.869% when pulling 36d504d on dalbani:COMPRESS-429 into 10ff1c3 on apache:master.

@coveralls
Copy link

coveralls commented Dec 12, 2017

Coverage Status

Coverage decreased (-0.02%) to 85.889% when pulling db0df3d on dalbani:COMPRESS-429 into 78cb9bf on apache:master.

@dalbani dalbani changed the title Provide information about presence of Unicode name / comment in ZipArchiveEntry [COMPRESS-429] Provide information about source of name / comment field values (e.g. Unicode) Dec 12, 2017
@coveralls
Copy link

coveralls commented Dec 12, 2017

Coverage Status

Coverage decreased (-0.04%) to 85.865% when pulling db0df3d on dalbani:COMPRESS-429 into 78cb9bf on apache:master.

@bodewig
Copy link
Member

bodewig commented Dec 27, 2017

Many thanks @dalbani this looks good. I've got two wishes, though:

  • could you please revert the change in imports to ZipFile? We don't want to use *-imports add all.
  • the new enums could use some javadoc inclusing a since marker

Of course, adding tests woudln't hurt , either;-)

@bodewig
Copy link
Member

bodewig commented Jan 5, 2018

@dalbani I've extended your PR with https://github.com/apache/commons-compress/tree/COMPRESS-429 - could you please have a look?

@@ -239,6 +239,7 @@ static void setNameAndCommentFromExtraFields(final ZipArchiveEntry ze,
originalNameBytes);
if (newName != null && !originalName.equals(newName)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I think about it, I would remove the second test in this if, so that the Unicode field always have "priority" over the original name as source.
Scenario where it would matter is when the originalName contains a non ASCII value (which is properly decoded) and which matches with the Unicode field value.

Copy link
Member

Choose a reason for hiding this comment

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

I think the second part is there for a reason, even though my memory is failing me right now - and I stumbled over it myself when I realized the same check is not present for the comment.

I understand what you mean, though, changed with c290609

Copy link
Member

Choose a reason for hiding this comment

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

After some digging through the history it seems the reason no longer exists, I've removed the second part with ced2075.

@dalbani
Copy link
Contributor Author

dalbani commented Jan 5, 2018

@bodewig: my apologies for not getting back to you sooner, I was on vacation.
What you wrote looks just fine to me — I would definitely not have done any better.
Kudos!
I have just added one comment though for a very specific use case.

@bodewig
Copy link
Member

bodewig commented Jan 7, 2018

Don't worry about any delay @dalbani many thanks.

I've merged the branch to master by now, will be included in Compress 1.16.

@asfgit asfgit closed this in 7cf10d7 Jan 7, 2018
@dalbani dalbani deleted the COMPRESS-429 branch January 7, 2018 20:38
@dalbani
Copy link
Contributor Author

dalbani commented Jan 7, 2018

@bodewig: thanks for your great overall reactivity for this feature request! I look forward for release 1.16.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants