Add support for arrays (and collections?) in type conversion #21

Closed
lviggiano opened this Issue Mar 28, 2013 · 4 comments

Comments

Projects
None yet
1 participant
@lviggiano
Owner

lviggiano commented Mar 28, 2013

Support for arrays and collections may be nice:

class MyConfig extends Config {
    @Separator({",", "|"})
    String[] strings();

    @Separator(File.pathSeparator);
    File[] path();

    List<Integer> ints();
}
@lviggiano

This comment has been minimized.

Show comment
Hide comment
@lviggiano

lviggiano May 21, 2013

Owner

Merged your change; thank you very much. I applied the minor changes that I commented on your patch, if you have any comment regarding my changes, let me know.

Before to finalize, we need to support array of primitives (missing at the moment); and provide an additional unit test to cover lines 131 and 147, see https://aeonbits.ci.cloudbees.com/job/owner-api/lastSuccessfulBuild/artifact/target/site/emma/_files/7.html

Line 147 on my laptop looks green btw; it's strange it's green on Jenkins. I'll check that too.

Owner

lviggiano commented May 21, 2013

Merged your change; thank you very much. I applied the minor changes that I commented on your patch, if you have any comment regarding my changes, let me know.

Before to finalize, we need to support array of primitives (missing at the moment); and provide an additional unit test to cover lines 131 and 147, see https://aeonbits.ci.cloudbees.com/job/owner-api/lastSuccessfulBuild/artifact/target/site/emma/_files/7.html

Line 147 on my laptop looks green btw; it's strange it's green on Jenkins. I'll check that too.

@lviggiano

This comment has been minimized.

Show comment
Hide comment
@lviggiano

lviggiano May 21, 2013

Owner

I investigated (using the debugger) why the lines 131 and 147 were reported as "uncovered" by emma.

    private Converters findAppropriateConverter(Class<?> targetType, String text) {
        for (Converters converter : values())
            if (converter.convert(targetType, text) != null)
                return converter;
        //line 131
        return UNSUPPORTED; // this is unreachable code, but the compiler needs it. 
    }

},

UNSUPPORTED {
    @Override
    Object convert(Class<?> targetType, String text) {
        //147
        throw new UnsupportedOperationException(String.format("Cannot convert '%s' to %s", text,
                targetType.getCanonicalName())); 
    }
};

the line 131 in method findAppropriateConverter is never reached because in the if(converter.convert(...)) it finally executes UNSUPPORTED.convert() which throws the exception; and this is correct. So the line 131, is never actually reached, but it is needed by the compiler, since it needs to return something. So it is fine that the coverage is not including this line.

about the line 147 instead, it is a peculiarity of emma tool, since the method always throws an exception, emma reports that the line is not covered (I feel this is a bug in emma) since the end of the method is not reached.

So, coverage is fine.

I added some "cosmetic" minor changes on the code to appeal my taste (style), and I moved the tests related to the arrays support in a new dedicated test class called ArraySupportTest.java

Thanks for adding the primitive arrays support. I really liked a lot the implementation you did for the Arrays that now also supports primitive types. Excellent job!

Luigi

Owner

lviggiano commented May 21, 2013

I investigated (using the debugger) why the lines 131 and 147 were reported as "uncovered" by emma.

    private Converters findAppropriateConverter(Class<?> targetType, String text) {
        for (Converters converter : values())
            if (converter.convert(targetType, text) != null)
                return converter;
        //line 131
        return UNSUPPORTED; // this is unreachable code, but the compiler needs it. 
    }

},

UNSUPPORTED {
    @Override
    Object convert(Class<?> targetType, String text) {
        //147
        throw new UnsupportedOperationException(String.format("Cannot convert '%s' to %s", text,
                targetType.getCanonicalName())); 
    }
};

the line 131 in method findAppropriateConverter is never reached because in the if(converter.convert(...)) it finally executes UNSUPPORTED.convert() which throws the exception; and this is correct. So the line 131, is never actually reached, but it is needed by the compiler, since it needs to return something. So it is fine that the coverage is not including this line.

about the line 147 instead, it is a peculiarity of emma tool, since the method always throws an exception, emma reports that the line is not covered (I feel this is a bug in emma) since the end of the method is not reached.

So, coverage is fine.

I added some "cosmetic" minor changes on the code to appeal my taste (style), and I moved the tests related to the arrays support in a new dedicated test class called ArraySupportTest.java

Thanks for adding the primitive arrays support. I really liked a lot the implementation you did for the Arrays that now also supports primitive types. Excellent job!

Luigi

@lviggiano

This comment has been minimized.

Show comment
Hide comment
@lviggiano

lviggiano May 23, 2013

Owner

Implementation is now complete. I just need to write some documentation with examples and check the javadocs in order to make sure things make sense.

Owner

lviggiano commented May 23, 2013

Implementation is now complete. I just need to write some documentation with examples and check the javadocs in order to make sure things make sense.

@ffbit ffbit referenced this issue May 23, 2013

Merged

Collection support #25

@lviggiano

This comment has been minimized.

Show comment
Hide comment
@lviggiano

lviggiano May 25, 2013

Owner

I wrote some documentation, I think everything is excellently implemented.

Owner

lviggiano commented May 25, 2013

I wrote some documentation, I think everything is excellently implemented.

@lviggiano lviggiano closed this May 25, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment