garydgregory commented on code in PR #655: URL: https://github.com/apache/commons-compress/pull/655#discussion_r2007521124
########## src/main/java/org/apache/commons/compress/archivers/zip/IZstdInputStreamCreator.java: ########## @@ -0,0 +1,35 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.commons.compress.archivers.zip; + +import java.io.InputStream; + +/** + * This interface can be implemented to support another Zstd compression library. + * Due the {@link ZipFile}s constructor is deprecated, this Interface can be used to pass it to the Builder. + */ +public interface IZstdInputStreamCreator { Review Comment: @mehmet-karaman You don't need a new type for this, use a `IOFunction`. Tangents: The usual naming convention for this pattern is "factory", not "creator". We do not use the "I" prefix for interfaces in this Commons component. All new public and protected elements should carry a Javadoc `since` tag. ########## src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java: ########## @@ -167,7 +168,26 @@ public ZipFile get() throws IOException { actualDescription = path.toString(); } final boolean closeOnError = seekableByteChannel != null; - return new ZipFile(actualChannel, actualDescription, getCharset(), useUnicodeExtraFields, closeOnError, ignoreLocalFileHeader); + return new ZipFile(actualChannel, actualDescription, getCharset(), useUnicodeExtraFields, closeOnError, ignoreLocalFileHeader) { + @Override + protected InputStream createZstdInputStream(InputStream is) throws IOException { + if (zstdInpStreamCreator != null) { + return zstdInpStreamCreator.createZstdInputStream(is); + } + return super.createZstdInputStream(is); + } + }; + } + + /** + * Sets the {@link IZstdInputStreamCreator} which gives the ability to use a different zstd compression library. + * @param zstdInpStreamCreator the {@link InputStream} creator for the other zstd compression library. + * @return {@code this} instance + * @since 1.28.0 + */ + public Builder setZstdInputStreamCreator(IZstdInputStreamCreator zstdInpStreamCreator) { Review Comment: The usual naming convention for this pattern is "factory", not a "creator". See above RE using a `Function`. reset to `ZstdCompressorInputStream::new` if null. ########## src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java: ########## @@ -167,7 +168,26 @@ public ZipFile get() throws IOException { actualDescription = path.toString(); } final boolean closeOnError = seekableByteChannel != null; - return new ZipFile(actualChannel, actualDescription, getCharset(), useUnicodeExtraFields, closeOnError, ignoreLocalFileHeader); + return new ZipFile(actualChannel, actualDescription, getCharset(), useUnicodeExtraFields, closeOnError, ignoreLocalFileHeader) { + @Override + protected InputStream createZstdInputStream(InputStream is) throws IOException { + if (zstdInpStreamCreator != null) { Review Comment: Do the null check in the setter and reset to `ZstdCompressorInputStream::new` if null. ########## src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java: ########## @@ -138,6 +138,7 @@ public static class Builder extends AbstractStreamBuilder<ZipFile, Builder> { private boolean useUnicodeExtraFields = true; private boolean ignoreLocalFileHeader; private long maxNumberOfDisks = 1; + private IZstdInputStreamCreator zstdInpStreamCreator; Review Comment: There is no need for a custom type or odd names parts like "Inp": ``` private IOFunction<InputStream,InputStream> zstdInputStream = ZstdCompressorInputStream::new; ``` ########## src/main/java/org/apache/commons/compress/archivers/zip/IZstdInputStreamCreator.java: ########## @@ -0,0 +1,35 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.commons.compress.archivers.zip; + +import java.io.InputStream; + +/** + * This interface can be implemented to support another Zstd compression library. + * Due the {@link ZipFile}s constructor is deprecated, this Interface can be used to pass it to the Builder. + */ +public interface IZstdInputStreamCreator { + + /** + * @param is the input stream which should be used for compression. Review Comment: This method Javadoc is missing its description. The method does not return "other libraries", it returns an input stream that supports decompressing the given Zstd-encoded input stream. ########## src/test/java/org/apache/commons/compress/archivers/zip/ZipFileTest.java: ########## @@ -66,8 +66,53 @@ import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.api.Test; +import io.airlift.compress.zstd.ZstdInputStream; + public class ZipFileTest extends AbstractTest { + private final class ZstdInputStreamCreator implements IZstdInputStreamCreator { + private boolean isUsed; Review Comment: Use the same naming convention as other test fixture: `isUsed` -> `used`. ########## src/test/java/org/apache/commons/compress/archivers/zip/ZipFileTest.java: ########## @@ -66,8 +66,53 @@ import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.api.Test; +import io.airlift.compress.zstd.ZstdInputStream; + public class ZipFileTest extends AbstractTest { + private final class ZstdInputStreamCreator implements IZstdInputStreamCreator { Review Comment: Add a Javadoc comment as to why we need a test class. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@commons.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org