Small refactoring in searching for attributes

Jiri Vanek jvanek at redhat.com
Wed Oct 30 09:49:56 PDT 2013


On 10/30/2013 05:19 PM, Omair Majid wrote:
> * Jiri Vanek <jvanek at redhat.com> [2013-10-30 07:14]:
>> On 10/26/2013 03:53 PM, Omair Majid wrote:
>>> I have mixed feelings about this. If you need to peek at a field to find
>>> out that this class is working correctly, maybe that fields needs to be
>>> public (or accessible through a getter/setter)?
>>
>> Nope. But I fixed it. Now the "main" is set in constructor so it is
>> as it should be. JArdesc immutability is correct, and field is set
>> during construction. Ideal. This is best on review process. One is
>> forced to read patch at least one more times :)
>
> Awesome!
>
>>>> +++ b/tests/netx/unit/net/sourceforge/jnlp/runtime/JNLPClassLoaderTest.java	Mon Oct 21 15:25:09 2013 +0200
>>>
>>>> +    public void getCustomAtributes() throws Exception {
>>>> +        File tempDirectory = FileTestUtils.createTempDirectory();
>>>> +        File jarLocation = new File(tempDirectory, "testX.jar");
>>>
>>> Can you decouple the unit test from the file system? The more this unit
>>> test does, the less useful it becomes as a 'unit' test (since the 'unit'
>>> now becomes 3 large classes and the file system).
>>
>> Sorry, not in time to investiage this. So no right now ;(
>
> Long term, this is bad. Maybe you want to hold onto this patch until you
> get time to sort it out completely?

please, not yet. from quick glance there is no easy solution :(
>
>>>> +        /* Test with attributes in manifest */ {
>>>
>>> When you feel tempted to write comments like this, it's an indication
>>> that it should be a separate unit test :)
>
> This is still not fixed. Is there any specific reason you want to stick
> to this style?


I must admit that I do not understand what you wont me to do :(
>
>>>> +      @Test
>>>> +    public void checkOrderWhenReadingAttributes() throws Exception {
>>>> +        File tempDirectory = FileTestUtils.createTempDirectory();
>>>> +        File jarLocation5 = new File(tempDirectory, "test5.jar");
>>>> +
>>>> +        /* Test with main-class in manifest */ {
>>>> +            Manifest manifest1 = new Manifest();
>>>> +            manifest1.getMainAttributes().put(Attributes.Name.MAIN_CLASS, "DummyClass1"); //two times, but one in main jar, see jnlpFile.setMainJar(3);
>>>
>>> Something is wrong in the indentation here. Please following the coding
>>> style: http://icedtea.classpath.org/wiki/IcedTea-Web#Code_style
>>
>> restyled
>
> The annotation's indentation is okay, but the indentation after the
> comment is still funny looking.

Crap will restyle again after previous comment is fixed

J.


More information about the distro-pkg-dev mailing list