Custom converter API and optional module with Java 8 Date&time converters #263

Merged
merged 5 commits into from Nov 1, 2016

Projects

None yet

4 participants

@hendrikebbers
Member
hendrikebbers commented Oct 25, 2016 edited

With this PR developers can provide custom converters for DP to provide more data types in the model.


This change is Reviewable

hendrikebbers added some commits Oct 24, 2016
@hendrikebbers hendrikebbers refactoring 1f691f9
@hendrikebbers hendrikebbers Converter SPI
type (int) matches to current JS version
3820bc2
@hendrikebbers hendrikebbers Date&Time converters
d2ffed3
@hendrikebbers hendrikebbers added this to the 0.8.8 milestone Oct 25, 2016
@coveralls

Coverage Status

Coverage increased (+0.6%) to 68.808% when pulling d2ffed3 on converter-export into b7e6780 on master.

@aalmiray
Contributor

Reviewed 45 of 45 files at r1.
Review status: all files reviewed at latest revision, 9 unresolved discussions.


platform-extras/dolphin-platform-date-time-converter/src/main/java/com/canoo/dolphin/converters/DurationConverterFactory.java, line 28 at r1 (raw file):

    private final static Converter CONVERTER = new DurationConverter();

    public final static int FIELD_TYPE = 51;

should this constant be placed at a common place with other constants of the same nature?


platform-extras/dolphin-platform-date-time-converter/src/main/java/com/canoo/dolphin/converters/LocalDateTimeConverterFactory.java, line 49 at r1 (raw file):

        @Override
        public LocalDateTime convertFromDolphin(String value) {

I'd rather have the method be named convertFromLiteral or convertFromString.


platform-extras/dolphin-platform-date-time-converter/src/main/java/com/canoo/dolphin/converters/LocalDateTimeConverterFactory.java, line 57 at r1 (raw file):

        @Override
        public String convertToDolphin(LocalDateTime value) {

perhaps rename to convertToLiteral or convertToString


platform-extras/dolphin-platform-date-time-converter/src/main/java/com/canoo/dolphin/converters/PeriodConverterFactory.java, line 55 at r1 (raw file):

                return null;
            }
            return Period.parse(value);

what happens if parse() throws DateTimeParseException because of an invalid value? Same thing with the other parsing converters


platform/dolphin-platform-core/src/main/java/com/canoo/dolphin/impl/BeanUtils.java, line 22 at r1 (raw file):

 */
public class BeanUtils {

if this class is still available then mark it as deprecated and route all of its methods to the corresponding DolphinUtils method.


platform/dolphin-platform-core/src/main/java/com/canoo/dolphin/impl/Converters.java, line 51 at r1 (raw file):

    public int getFieldType(Class<?> clazz) {
        return getFactory(clazz).getTypeIdentifier();

what if there's no match for clazz? There may be an NPE


platform/dolphin-platform-core/src/main/java/com/canoo/dolphin/impl/Converters.java, line 55 at r1 (raw file):

    public Converter getConverter(Class<?> clazz) {
        return getFactory(clazz).getConverterForType(clazz);

possible NPE. see above


platform/dolphin-platform-core/src/main/java/com/canoo/dolphin/impl/Converters.java, line 67 at r1 (raw file):

        }
        if (foundConverters.size() > 1) {
            throw new RuntimeException("More than 1 converter instance found to convert " + clazz);

use another exception type


platform/dolphin-platform-core/src/main/java/com/canoo/dolphin/impl/Converters.java, line 70 at r1 (raw file):

        }
        if (foundConverters.isEmpty()) {
            throw new RuntimeException("No converter instance found to convert " + clazz);

use another exception type


Comments from Reviewable

@hendrikebbers hendrikebbers documentation
a771ae3
@coveralls

Coverage Status

Coverage increased (+0.7%) to 68.829% when pulling a771ae3 on converter-export into b7e6780 on master.

@hendrikebbers hendrikebbers Based on review
ac927cc
@hendrikebbers
Member

Review status: 14 of 55 files reviewed at latest revision, 9 unresolved discussions.


platform-extras/dolphin-platform-date-time-converter/src/main/java/com/canoo/dolphin/converters/DurationConverterFactory.java, line 28 at r1 (raw file):

Previously, aalmiray (Andres Almiray) wrote…

should this constant be placed at a common place with other constants of the same nature?

Done

platform-extras/dolphin-platform-date-time-converter/src/main/java/com/canoo/dolphin/converters/LocalDateTimeConverterFactory.java, line 49 at r1 (raw file):

Previously, aalmiray (Andres Almiray) wrote…

I'd rather have the method be named convertFromLiteral or convertFromString.

mmh, is "literal" right? Because it will cover String, boolean & number. German translation in dictionary looks like it's only for strings. If you think it's right we can use it. Maybe "convertFromRaw" ?

platform-extras/dolphin-platform-date-time-converter/src/main/java/com/canoo/dolphin/converters/PeriodConverterFactory.java, line 55 at r1 (raw file):

Previously, aalmiray (Andres Almiray) wrote…

what happens if parse() throws DateTimeParseException because of an invalid value? Same thing with the other parsing converters

Introduced ValueConverterException

platform/dolphin-platform-core/src/main/java/com/canoo/dolphin/impl/BeanUtils.java, line 22 at r1 (raw file):

Previously, aalmiray (Andres Almiray) wrote…

if this class is still available then mark it as deprecated and route all of its methods to the corresponding DolphinUtils method.

removed

platform/dolphin-platform-core/src/main/java/com/canoo/dolphin/impl/Converters.java, line 51 at r1 (raw file):

Previously, aalmiray (Andres Almiray) wrote…

what if there's no match for clazz? There may be an NPE

no, check is in getFactory(Class)

platform/dolphin-platform-core/src/main/java/com/canoo/dolphin/impl/Converters.java, line 55 at r1 (raw file):

Previously, aalmiray (Andres Almiray) wrote…

possible NPE. see above

no, check is in getFactory(Class)

Comments from Reviewable

@hendrikebbers
Member

Review status: 14 of 55 files reviewed at latest revision, 9 unresolved discussions.


platform/dolphin-platform-core/src/main/java/com/canoo/dolphin/impl/Converters.java, line 67 at r1 (raw file):

Previously, aalmiray (Andres Almiray) wrote…

use another exception type

done

platform/dolphin-platform-core/src/main/java/com/canoo/dolphin/impl/Converters.java, line 70 at r1 (raw file):

Previously, aalmiray (Andres Almiray) wrote…

use another exception type

done

Comments from Reviewable

@coveralls

Coverage Status

Coverage increased (+0.2%) to 68.363% when pulling ac927cc on converter-export into b7e6780 on master.

@hendrikebbers hendrikebbers merged commit 9b119a4 into master Nov 1, 2016

3 of 4 checks passed

code-review/reviewable 41 files, 9 discussions left (aalmiray, hendrikebbers)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 68.363%
Details
@hendrikebbers hendrikebbers deleted the converter-export branch Nov 1, 2016
@aalmiray aalmiray removed the in progress label Nov 1, 2016
@netopyr
Collaborator
netopyr commented Nov 1, 2016

platform-extras/dolphin-platform-date-time-converter/src/main/java/com/canoo/dolphin/converters/LocalDateTimeConverterFactory.java, line 49 at r1 (raw file):

Previously, hendrikebbers (Hendrik Ebbers) wrote…

mmh, is "literal" right? Because it will cover String, boolean & number. German translation in dictionary looks like it's only for strings. If you think it's right we can use it. Maybe "convertFromRaw" ?

I'd say literal has a different meaning. String is not sufficient. Not sure if raw is really better. The point here is, that a converter converts values from the application domain to something that OpenDolphin can handle and back. Currently this is defined by the capabilities of JSON, therefore my first attempt was convertFromJSON and convertToJSON. But we do not necessarily keep JSON, and these names are inappropriate, too.

I think whatever name you choose, it should not be bound to the current implementation, but should also make sense with other remoting components.


Comments from Reviewable

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