garydgregory commented on code in PR #455: URL: https://github.com/apache/commons-compress/pull/455#discussion_r1451528565
########## src/main/java/org/apache/commons/compress/archivers/zip/ZipOpenOptions.java: ########## @@ -0,0 +1,286 @@ +/* + * 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 + * + * http://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.nio.channels.SeekableByteChannel; +import java.nio.charset.Charset; +import java.nio.charset.StandardCharsets; +import java.nio.file.Path; +import java.util.Objects; + + +/** + * Zip open archive options. The class is constructed via builder. + */ +public class ZipOpenOptions { + private final Path path; + + private final SeekableByteChannel channel; + + private final Charset encoding; + + private final boolean useUnicodeExtraFields; + + private final boolean ignoreLocalFileHeader; + + private final long maxNumberOfDisks; + + /** + * Gets archive path. + * + * @return + * archive path + */ + public Path path() { + return path; + } + + /** + * Gets archive file channel. + * + * @return + * archive file channel + */ + public SeekableByteChannel channel() { + return channel; + } + + /** + * Gets archive encoding. + * + * @return + * archive encoding + */ + public Charset encoding() { + return encoding; + } + + /** + * Indicates whether to use unicode extra fields, default is true. + * + * @return + * indicates whether to use unicode extra fields + */ + public boolean useUnicodeExtraFields() { + return useUnicodeExtraFields; + } + + /** + * Indicates whether to ignore local headers, default is false. + * + * @return + * indicates whether to ignore local headers. + */ + public boolean ignoreLocalFileHeader() { + return ignoreLocalFileHeader; + } + + /** + * Gets max number of multi archive disks, default is 1 (no multi archive). + * + * @return + * max number of multi archive disks + */ + public long maxNumberOfDisks() { + return maxNumberOfDisks; + } + + /** + * Gets whether to close the channel on error during opening, default is true. + * + * @return + * whether to close the channel on error during opening + */ + public boolean closeOnError() { + return closeOnError; + } + + private final boolean closeOnError; + + /** + * Creates new Builder. + * + * @return + * new freshly initialized Builder. + */ + public static Builder builder() { + return new Builder(); + } + + /** + * Converts the object to mutable builder. + * + * @return + * builder with same configuration. + */ + public Builder toBuilder() { + return builder() + .path(path) + .channel(channel) + .encoding(encoding) + .useUnicodeExtraFields(useUnicodeExtraFields) + .ignoreLocalFileHeader(ignoreLocalFileHeader) + .maxNumberOfDisks(maxNumberOfDisks) + .closeOnError(closeOnError); + } + + /** + * Creates new instance. The instance is not thread safe. + */ + protected ZipOpenOptions(Builder builder) { + this.path = builder.path; + this.channel = builder.channel; + this.encoding = builder.encoding; + this.useUnicodeExtraFields = builder.useUnicodeExtraFields; + this.ignoreLocalFileHeader = builder.ignoreLocalFileHeader; + this.maxNumberOfDisks = builder.maxNumberOfDisks; + this.closeOnError = builder.closeOnError; + } + + public static class Builder { + + private Path path; + + private SeekableByteChannel channel; + + private Charset encoding = StandardCharsets.UTF_8; + + private boolean useUnicodeExtraFields = true; + + private boolean ignoreLocalFileHeader = false; Review Comment: Don't initialize instance variables to their default value. ########## src/main/java/org/apache/commons/compress/archivers/zip/ZipOpenOptions.java: ########## @@ -0,0 +1,286 @@ +/* + * 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 + * + * http://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.nio.channels.SeekableByteChannel; +import java.nio.charset.Charset; +import java.nio.charset.StandardCharsets; +import java.nio.file.Path; +import java.util.Objects; + + +/** + * Zip open archive options. The class is constructed via builder. + */ +public class ZipOpenOptions { + private final Path path; + + private final SeekableByteChannel channel; + + private final Charset encoding; + + private final boolean useUnicodeExtraFields; + + private final boolean ignoreLocalFileHeader; + + private final long maxNumberOfDisks; + + /** + * Gets archive path. + * + * @return + * archive path + */ + public Path path() { + return path; + } + + /** + * Gets archive file channel. + * + * @return + * archive file channel + */ + public SeekableByteChannel channel() { + return channel; + } + + /** + * Gets archive encoding. + * + * @return + * archive encoding + */ + public Charset encoding() { + return encoding; + } + + /** + * Indicates whether to use unicode extra fields, default is true. + * + * @return + * indicates whether to use unicode extra fields + */ + public boolean useUnicodeExtraFields() { + return useUnicodeExtraFields; + } + + /** + * Indicates whether to ignore local headers, default is false. + * + * @return + * indicates whether to ignore local headers. + */ + public boolean ignoreLocalFileHeader() { + return ignoreLocalFileHeader; + } + + /** + * Gets max number of multi archive disks, default is 1 (no multi archive). + * + * @return + * max number of multi archive disks + */ + public long maxNumberOfDisks() { + return maxNumberOfDisks; + } + + /** + * Gets whether to close the channel on error during opening, default is true. + * + * @return + * whether to close the channel on error during opening + */ + public boolean closeOnError() { + return closeOnError; + } + + private final boolean closeOnError; + + /** + * Creates new Builder. + * + * @return + * new freshly initialized Builder. + */ + public static Builder builder() { + return new Builder(); + } + + /** + * Converts the object to mutable builder. + * + * @return + * builder with same configuration. + */ + public Builder toBuilder() { + return builder() + .path(path) + .channel(channel) + .encoding(encoding) + .useUnicodeExtraFields(useUnicodeExtraFields) + .ignoreLocalFileHeader(ignoreLocalFileHeader) + .maxNumberOfDisks(maxNumberOfDisks) + .closeOnError(closeOnError); + } + + /** + * Creates new instance. The instance is not thread safe. + */ + protected ZipOpenOptions(Builder builder) { + this.path = builder.path; + this.channel = builder.channel; + this.encoding = builder.encoding; + this.useUnicodeExtraFields = builder.useUnicodeExtraFields; + this.ignoreLocalFileHeader = builder.ignoreLocalFileHeader; + this.maxNumberOfDisks = builder.maxNumberOfDisks; + this.closeOnError = builder.closeOnError; + } + + public static class Builder { + + private Path path; + + private SeekableByteChannel channel; + + private Charset encoding = StandardCharsets.UTF_8; + + private boolean useUnicodeExtraFields = true; + + private boolean ignoreLocalFileHeader = false; + + private long maxNumberOfDisks = 1; + + private boolean closeOnError = true; + + /** + * Builds instance of ZipOpenOptions from configured values. + * + * @return + * ZipOpenOptions immutable instance. + */ + public ZipOpenOptions build() { + return new ZipOpenOptions(this); + } + + /** + * Sets archive path. + * + * @param path + * archive path + * + * @return this instance + */ + public ZipOpenOptions.Builder path(Path path) { Review Comment: Use "set" as the prefix for all setter methods. ########## src/main/java/org/apache/commons/compress/archivers/zip/ZipOpenOptions.java: ########## @@ -0,0 +1,286 @@ +/* + * 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 + * + * http://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.nio.channels.SeekableByteChannel; +import java.nio.charset.Charset; +import java.nio.charset.StandardCharsets; +import java.nio.file.Path; +import java.util.Objects; + + +/** + * Zip open archive options. The class is constructed via builder. + */ Review Comment: Javadoc: Add `@since 1.26.0` for all new public and protected elements. ########## src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java: ########## @@ -435,6 +444,39 @@ public static void closeQuietly(final ZipFile zipFile) { private long firstLocalFileHeaderOffset; + + public ZipFile(ZipOpenOptions options) throws IOException { Review Comment: This class already has 15 (!) constructors, I want to avoid even more constructor creep. Instead of a builder of options, Commons IO provides the ability to provide builders for IO classes based on various IO elements like File, Path, and streams. Let's refactor this class like this example of another class with an exploding constructor count: [FileWriterWithEncoding](https://github.com/apache/commons-io/blob/master/src/main/java/org/apache/commons/io/output/FileWriterWithEncoding.java) I'll work on that... -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
