garydgregory commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r774501046
##########
File path:
src/main/java/org/apache/commons/compress/archivers/zip/ScatterZipOutputStream.java
##########
@@ -198,7 +210,19 @@ public static ScatterZipOutputStream fileBased(final File
file) throws FileNotFo
* @throws FileNotFoundException if the file cannot be found
*/
public static ScatterZipOutputStream fileBased(final File file, final int
compressionLevel) throws FileNotFoundException {
- final ScatterGatherBackingStore bs = new
FileBasedScatterGatherBackingStore(file);
+ return pathBased(file.toPath(), compressionLevel);
+ }
+
+ /**
+ * Create a {@link ScatterZipOutputStream} that is backed by a file
+ *
+ * @param path The path to offload compressed data into.
+ * @param compressionLevel The compression level to use, @see #Deflater
+ * @return A ScatterZipOutputStream that is ready for use.
+ * @throws FileNotFoundException if the path cannot be found
+ */
Review comment:
Missing since tag on new or protected method.
##########
File path:
src/main/java/org/apache/commons/compress/archivers/zip/ZipArchiveOutputStream.java
##########
@@ -518,6 +543,11 @@ public void setUseZip64(final Zip64Mode mode) {
zip64Mode = mode;
}
+ @Override
Review comment:
Missing since tag on new or protected method.
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java
##########
@@ -210,7 +222,21 @@ public ZipFile(final String name, final String encoding)
throws IOException {
* @throws IOException if an error occurs while reading the file.
*/
public ZipFile(final File f, final String encoding) throws IOException {
- this(f, encoding, true);
+ this(f.toPath(), encoding, true);
+ }
+
+ /**
+ * Opens the given path for reading, assuming the specified
+ * encoding for file names and scanning for unicode extra fields.
+ *
+ * @param path path to the archive.
+ * @param encoding the encoding to use for file names, use null
+ * for the platform's default encoding
+ *
+ * @throws IOException if an error occurs while reading the file.
+ */
Review comment:
Missing since tag on new or protected method.
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java
##########
@@ -227,7 +253,24 @@ public ZipFile(final File f, final String encoding) throws
IOException {
*/
public ZipFile(final File f, final String encoding, final boolean
useUnicodeExtraFields)
throws IOException {
- this(f, encoding, useUnicodeExtraFields, false);
+ this(f.toPath(), encoding, useUnicodeExtraFields, false);
+ }
+
+ /**
+ * Opens the given path for reading, assuming the specified
+ * encoding for file names.
+ *
+ * @param path path to the archive.
+ * @param encoding the encoding to use for file names, use null
+ * for the platform's default encoding
+ * @param useUnicodeExtraFields whether to use InfoZIP Unicode
+ * Extra Fields (if present) to set the file names.
Review comment:
Ditto
##########
File path:
src/test/java/org/apache/commons/compress/archivers/zip/ScatterZipOutputStreamTest.java
##########
@@ -45,7 +46,8 @@ public void cleanup() {
@Test
public void putArchiveEntry() throws Exception {
scatterFile = File.createTempFile("scattertest", ".notzip");
- final ScatterZipOutputStream scatterZipOutputStream =
ScatterZipOutputStream.fileBased(scatterFile);
+ final ScatterZipOutputStream scatterZipOutputStream =
ScatterZipOutputStream.fileBased(scatterFile,
+ Deflater.BEST_COMPRESSION);
final byte[] B_PAYLOAD = "RBBBBBBS".getBytes();
Review comment:
Hm, why? How is this part of the NIO change? Hard to te with so many
changes...
##########
File path:
src/main/java/org/apache/commons/compress/archivers/zip/ZipSplitReadOnlySeekableByteChannel.java
##########
@@ -58,11 +61,11 @@
*
* @param channels the channels to concatenate
* @throws NullPointerException if channels is null
- * @throws IOException if the first channel doesn't seem to hold
- * the beginning of a split archive
+ * @throws IOException if the first channel doesn't seem to hold
+ * the beginning of a split archive
Review comment:
Ditto
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java
##########
@@ -262,6 +305,39 @@ public ZipFile(final File f, final String encoding, final
boolean useUnicodeExtr
f.getAbsolutePath(), encoding, useUnicodeExtraFields, true,
ignoreLocalFileHeader);
}
+ /**
+ * Opens the given path for reading, assuming the specified
+ * encoding for file names.
+ *
+ *
Review comment:
Ditto
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java
##########
@@ -210,7 +222,21 @@ public ZipFile(final String name, final String encoding)
throws IOException {
* @throws IOException if an error occurs while reading the file.
*/
public ZipFile(final File f, final String encoding) throws IOException {
- this(f, encoding, true);
+ this(f.toPath(), encoding, true);
+ }
+
+ /**
+ * Opens the given path for reading, assuming the specified
+ * encoding for file names and scanning for unicode extra fields.
+ *
+ * @param path path to the archive.
+ * @param encoding the encoding to use for file names, use null
+ * for the platform's default encoding
Review comment:
No need for extra whitespace.
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java
##########
@@ -174,6 +175,17 @@ public ZipFile(final File f) throws IOException {
this(f, ZipEncodingHelper.UTF8);
}
+ /**
+ * Opens the given path for reading, assuming "UTF8" for file names.
+ *
+ * @param path path to the archive.
+ *
+ * @throws IOException if an error occurs while reading the file.
+ */
+ public ZipFile(final Path path) throws IOException {
Review comment:
Missing since tag on new or protected method.
##########
File path:
src/main/java/org/apache/commons/compress/archivers/zip/ZipSplitOutputStream.java
##########
@@ -57,19 +58,31 @@
* @param zipFile the zip file to write to
* @param splitSize the split size
*/
- public ZipSplitOutputStream(final File zipFile, final long splitSize)
throws IllegalArgumentException, IOException {
+ public ZipSplitOutputStream(final Path zipFile, final long splitSize)
throws IllegalArgumentException, IOException {
if (splitSize < ZIP_SEGMENT_MIN_SIZE || splitSize >
ZIP_SEGMENT_MAX_SIZE) {
throw new IllegalArgumentException("zip split segment size should
between 64K and 4,294,967,295");
}
this.zipFile = zipFile;
this.splitSize = splitSize;
- this.outputStream = Files.newOutputStream(zipFile.toPath());
+ this.outputStream = Files.newOutputStream(zipFile);
// write the zip split signature 0x08074B50 to the zip file
writeZipSplitSignature();
}
+ /**
+ * Create a split zip. If the zip file is smaller than the split size,
+ * then there will only be one split zip, and its suffix is .zip,
+ * otherwise the split segments should be like .z01, .z02, ... .z(N-1),
.zip
+ *
+ * @param zipFile the zip file to write to
+ * @param splitSize the split size
+ */
Review comment:
Missing since tag on new or protected method.
##########
File path:
src/main/java/org/apache/commons/compress/archivers/zip/ZipArchiveOutputStream.java
##########
@@ -365,8 +365,33 @@ public ZipArchiveOutputStream(final Path file, final
OpenOption... options) thro
* @since 1.20
*/
public ZipArchiveOutputStream(final File file, final long zipSplitSize)
throws IOException {
+ this(file.toPath(), zipSplitSize);
+ }
+
+ /**
+ * Creates a split ZIP Archive.
+ *
+ * <p>The files making up the archive will use Z01, Z02,
+ * ... extensions and the last part of it will be the given {@code
+ * file}.</p>
+ *
+ * <p>Even though the stream writes to a file this stream will
+ * behave as if no random access was possible. This means the
+ * sizes of stored entries need to be known before the actual
+ * entry data is written.</p>
+ *
+ * @param path the file that will become the last part of the split archive
+ * @param zipSplitSize maximum size of a single part of the split
+ * archive created by this stream. Must be between 64kB and about
+ * 4GB.
+ *
Review comment:
No need for extra whitespace.
##########
File path:
src/main/java/org/apache/commons/compress/archivers/zip/ZipSplitReadOnlySeekableByteChannel.java
##########
@@ -58,11 +61,11 @@
*
* @param channels the channels to concatenate
* @throws NullPointerException if channels is null
- * @throws IOException if the first channel doesn't seem to hold
- * the beginning of a split archive
+ * @throws IOException if the first channel doesn't seem to hold
+ * the beginning of a split archive
*/
public ZipSplitReadOnlySeekableByteChannel(final List<SeekableByteChannel>
channels)
- throws IOException {
+ throws IOException {
Review comment:
Don't change formatting.
##########
File path:
src/main/java/org/apache/commons/compress/archivers/zip/ZipSplitReadOnlySeekableByteChannel.java
##########
@@ -47,7 +50,7 @@
public class ZipSplitReadOnlySeekableByteChannel extends
MultiReadOnlySeekableByteChannel {
private static final int ZIP_SPLIT_SIGNATURE_LENGTH = 4;
private final ByteBuffer zipSplitSignatureByteBuffer =
- ByteBuffer.allocate(ZIP_SPLIT_SIGNATURE_LENGTH);
+ ByteBuffer.allocate(ZIP_SPLIT_SIGNATURE_LENGTH);
Review comment:
Please do not change the formatting, it makes reviewing harder and take
more time.
##########
File path: src/main/java/org/apache/commons/compress/utils/FileNameUtils.java
##########
@@ -50,6 +51,19 @@ public static String getExtension(final String filename) {
return name.substring(extensionPosition + 1);
}
+ public static String getExtensionFrom(final Path path) {
Review comment:
Make this private or package private. The fewer public or protected
method the better as they will have to be maintained "forever". Also needs a
comment.
##########
File path:
src/test/java/org/apache/commons/compress/archivers/zip/ZipSplitOutputStreamTest.java
##########
@@ -69,19 +69,19 @@ public void splitZipBeginsWithZipSplitSignature() throws
IOException {
public void testCreateSplittedFiles() throws IOException {
final File testOutputFile = new File(dir,
"testCreateSplittedFiles.zip");
final int splitSize = 100 * 1024; /* 100KB */
- final ZipSplitOutputStream zipSplitOutputStream = new
ZipSplitOutputStream(testOutputFile, splitSize);
+ final ZipSplitOutputStream ZipSplitOutputStream = new
ZipSplitOutputStream(testOutputFile, splitSize);
Review comment:
-1: Local var names never start with a CAPITAL letter.
##########
File path:
src/main/java/org/apache/commons/compress/archivers/zip/ZipSplitReadOnlySeekableByteChannel.java
##########
@@ -108,10 +111,10 @@ private void assertSplitSignature(final
List<SeekableByteChannel> channels)
* Concatenates the given channels.
*
* @param channels the channels to concatenate, note that the LAST CHANNEL
of channels should be the LAST SEGMENT(.zip)
- * and theses channels should be added in correct order
(e.g. .z01, .z02... .z99, .zip)
+ * and these channels should be added in correct order
(e.g.: .z01, .z02... .z99, .zip)
* @return SeekableByteChannel that concatenates all provided channels
* @throws NullPointerException if channels is null
- * @throws IOException if reading channels fails
+ * @throws IOException if reading channels fails
Review comment:
Ditto
##########
File path: src/main/java/org/apache/commons/compress/utils/FileNameUtils.java
##########
@@ -75,4 +89,19 @@ public static String getBaseName(final String filename) {
return name.substring(0, extensionPosition);
}
+
+ public static String getBaseNameFrom(final Path path) {
Review comment:
As above plus needs a comment.
##########
File path:
src/test/java/org/apache/commons/compress/archivers/zip/ZipSplitOutputStreamTest.java
##########
@@ -69,19 +69,19 @@ public void splitZipBeginsWithZipSplitSignature() throws
IOException {
public void testCreateSplittedFiles() throws IOException {
final File testOutputFile = new File(dir,
"testCreateSplittedFiles.zip");
final int splitSize = 100 * 1024; /* 100KB */
- final ZipSplitOutputStream zipSplitOutputStream = new
ZipSplitOutputStream(testOutputFile, splitSize);
+ final ZipSplitOutputStream ZipSplitOutputStream = new
ZipSplitOutputStream(testOutputFile, splitSize);
final File fileToTest =
getFile("COMPRESS-477/split_zip_created_by_zip/zip_to_compare_created_by_zip.zip");
final InputStream inputStream =
Files.newInputStream(fileToTest.toPath());
final byte[] buffer = new byte[4096];
int readLen;
while ((readLen = inputStream.read(buffer)) > 0) {
- zipSplitOutputStream.write(buffer, 0, readLen);
+ ZipSplitOutputStream.write(buffer, 0, readLen);
Review comment:
-1 ditto
##########
File path:
src/main/java/org/apache/commons/compress/archivers/zip/ZipSplitReadOnlySeekableByteChannel.java
##########
@@ -125,14 +128,14 @@ public static SeekableByteChannel
forOrderedSeekableByteChannels(final SeekableB
*
* @param lastSegmentChannel channel of the last segment of split zip
segments, its extension should be .zip
* @param channels the channels to concatenate except for the
last segment,
- * note theses channels should be added in
correct order (e.g. .z01, .z02... .z99)
+ * note these channels should be added in
correct order (e.g.: .z01, .z02... .z99)
* @return SeekableByteChannel that concatenates all provided channels
* @throws NullPointerException if lastSegmentChannel or channels is null
- * @throws IOException if the first channel doesn't seem to hold
- * the beginning of a split archive
+ * @throws IOException if the first channel doesn't seem to hold
+ * the beginning of a split archive
*/
public static SeekableByteChannel forOrderedSeekableByteChannels(final
SeekableByteChannel lastSegmentChannel,
- final Iterable<SeekableByteChannel> channels) throws IOException {
+ final
Iterable<SeekableByteChannel> channels) throws IOException {
Review comment:
Again, change messes up formatting.
--
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
