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

Add encoding property for zip task #405

Closed
wants to merge 0 commits into from

Conversation

AlphaHinex
Copy link
Contributor

This pr could fix problems such as chinese characters in file name change to garbled characters after using zip type task

@ghale
Copy link
Member

ghale commented Feb 18, 2015

Thanks for the pull request. We'll have someone look at it.

In the meantime, can you sign the CLA online at www.gradleware.com/contributor-agreement?

@AlphaHinex
Copy link
Contributor Author

ok, I've signed it

@bigdaz
Copy link
Member

bigdaz commented Mar 3, 2015

Hi. A few things this functionality would require:

  1. An integration test covering non-ascii characters
  2. A public way to configure the encoding on a Zip task

Let me know if you want to help out with this.

@ldaley
Copy link
Member

ldaley commented Mar 3, 2015

@bigdaz I'm not sure we'd need to make this configurable. We should look into making this the default. This would be doable if the current value is always a UTF-8 subset. It's probably not possible if the current value is dependent on platform encoding.

@AlphaHinex
Copy link
Contributor Author

@bigdaz I prefer configurable way too, and I will do this next.

@alkemist I think somebody like me will need this feature in some case. For example, the platform encoding of chinese version Windows is GBK by default, and the files with chinese characters in name could not unpack from war archive if not set zip task encoding to UTF-8.

@ldaley
Copy link
Member

ldaley commented Mar 4, 2015

@AlphaHinex we don't know what encoding is currently being used. If the current value is the platform encoding, then we can't change to UTF-8. If it however is hardcoded to US-ASCII, then we can.

@AlphaHinex
Copy link
Contributor Author

@alkemist so we do not hardcode it, we use the platform encoding by default and give user a way to configure it, is it a good idea?

@ldaley
Copy link
Member

ldaley commented Mar 9, 2015

From the docs of ZipOutputStream.setEncoding

Defaults to the platform's default character encoding.

This makes defaulting/hardcoding to UTF-8 a breaking change, but we might be able to live with it.

Options:

  1. Hardcode this to UTF-8 (i.e. what this PR does)
  2. Add an encoding property to Zip that defaults to platform encoding
  3. Add an encoding property to Zip that defaults to UTF-8

2 is backwards compatible. 3 is a better default, but potentially breaking.

My preference is 3.

@adammurdoch ?

@mark-vieira
Copy link
Contributor

@AlphaHinex Do you mind moving forward on implementing option # 3 above?

@AlphaHinex
Copy link
Contributor Author

OK, I will try

@AlphaHinex AlphaHinex changed the title Set encoding explicitly for zip task Add encoding property for zip task Jun 18, 2015
@AlphaHinex
Copy link
Contributor Author

Sorry for the long time, but during these days, I've changed my mind. I just add an encoding property for Zip task, and leave it defaults to platform encoding, option # 2 above.

def theZip = new ZipTestFixture(file('build/test.zip'), encoding)
theZip.hasDescendants("${filename}1.txt", "${filename}2.txt", "${filename}3.txt")
}

Copy link
Member

Choose a reason for hiding this comment

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

Should use Spock @Unroll here to avoid test duplication. Would also be useful to have a test explicitly setting null encoding.

@bigdaz
Copy link
Member

bigdaz commented Aug 6, 2015

Thanks @AlphaHinex. I think that you are correct to go with keeping the current default behaviour, and making it possible to override with the encoding option.

Before we can merge, we'd need some test coverage for explicitly setting null encoding, as well as a test for failure with an invalid encoding value.

I think the best way to proceed is to close this PR, and open a new one with just the changes required.

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.

5 participants