RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start

Lance Andersen lance.andersen at oracle.com
Thu Mar 5 12:35:52 UTC 2020

Hi Christoph

> On Mar 5, 2020, at 4:03 AM, Langer, Christoph <christoph.langer at sap.com> wrote:
> Hi Lance,
> Thanks for addressing my points. Here my answer to those things which weren't in full agreement yet:

Please see below
>> I dislike a bit the fact that we introduce a new testng subfolder in
>> test/jdk/nio/zipfs. Wouldn’t it be possible to consolidate in the current test
>> folder?
>> I am trying to add some organization separating the non-testng  tests from
>> the  testng tests and other testng tests will be moved here.  I did the same
>> for JDBC a few years back
> ok, maybe it's a good idea to do this here and gradually move all testng tests over.
>> - line 387: Manfiest -> Manifest
> I think you missed that one

Fixed must have not saved it but it is there now
>> - line 417: Parameter "final Map<?, ?>
>> attributes" of ManifestOrderTest::verify should be renamed to
>> "manifestAttributes" to make it easier to understand its purpose
>> Prefer to leave as is as it gets wordy and I believe the description is clear in
>> the comments
> Hm, I needed to look twice to grasp it. So, I'd still prefer something with "manifest" in the variable name. But I leave it to you since it's probably a personal taste thing ��

I left the variable name as to your point it sometimes is a personal preference ;-) 
> However, two more things here:
> The Javadoc of verify says (line 412):
> * @param attributes  Is there a Manifest to check
> You should rather go with the Javadoc of validateManifest here as well:
> * @param attributes A Map containing the attributes expected in the Manifest;
> *                   otherwise empty

Updated…  Thought I had done that previously as that @param was originally a boolean ….
> Also, I spotted in the Javadoc, line 413:
> * @param entries     Entries to validate are in the JAR
> I guess the "are" is wrong here.


>> test/jdk/jdk/nio/zipfs/testng/util/ZipFsBaseTest.java:
>> - rename to ZipFSBaseTest (Capital ‘S’)??
>> hmm  I had that originally but did not like it…  but don’t have a strong
>> preference
> Ok, leave it as you have it ��

>> - line 120: public static void verify - > this method is not used by
>> ManifestOrderTest. I think we should only add it when having a test that
>> really uses this verify method. But I generally agree that the verify method is
>> a candidate for communalization
>> This will be used by some tests I have created and some I will be moving so
>> rather add it now on the initial push.  This is used by several tests that will be
>> migrated
>> - line 176: public void zip: dito, this method doesn’t seem used. So I suggest
>> to remove it for this change
>> Same comment as above
> OK.
>> The webrev for the above
>> is http://cr.openjdk.java.net/~lancea/8211917/webrev.01/index.html
> Looks good, apart from my comments above.

Thank you again for the review

The revised webrev is at http://cr.openjdk.java.net/~lancea/8211917/webrev.02/index.html <http://cr.openjdk.java.net/~lancea/8211917/webrev.02/index.html>

> Thanks
> Christoph

 <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>
 <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
Lance.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com>

More information about the core-libs-dev mailing list