Skip to content

Conversation

@hinerm
Copy link
Member

@hinerm hinerm commented Sep 18, 2014

Improves the Converter framework by moving more default method implementations to AbstractConverter, restructuring the delegation of AbstractConverter to give preference to the Type argument, and extracting the null parameter logic to a dedicated NullConverter which can run at the highest priority, so that other Converter implementations do not need to worry about running ahead of DefaultConverter.

The AbstractConverter class was leaving a lot of methods unimplemented.
Typically, most of these can be chained together.

The AbstractConverter now implements all but one convert and canConvert
signature.
@hinerm
Copy link
Member Author

hinerm commented Sep 18, 2014

@dscho @ctrueden @dietzc - these changes are intended to make it easier to implement new Converters. Please let me know if you have any reservations or further suggestions.

@ctrueden
Copy link
Member

Thanks @hinerm! I made some comments. Please address as time allows.

One other thing to consider would be deprecating the Class-based signatures in favor of only Type-based ones. (Because a Class is a Type, so you can still use them that way.) And the AbstractConverter layer could have a protected Class<?> raw(Type type) method that just returns GenericUtils.getClass(type) for maximum convenience. I guess the only downside would be that Type has no generic parameter, so the <T> T convert(Object, Class<T>) signature would become less type-safe... but we could still keep that one undeprecated then.

hinerm and others added 4 commits September 23, 2014 13:52
Removed unnecessary implementations for dummy classes, as these methods
are now taken care of in the abstract layer.
Adds a new Converter type to handle null source or destination types.

Without this converter, the null logic was managed in a mix between
AbstractConverter and DefaultConverter - which means that all converters
had some null logic in them (potentially wrong for that given converter)
and could not run before DefaultConverter, without copying its null
logic.
Ensure the null converter is behaving as intended.
Refactors AbstractConverter and DefaultConverter to take into account
the new NullConverter and the migration of boilerplate to
AbstractConverter.

Key points:
* Control flow at the abstract layer attempts to preserve the Type if
  given, as this contains information that can be lost in a Class.
* Removed several delegating method implementations from DefaultConverter
@hinerm
Copy link
Member Author

hinerm commented Sep 23, 2014

TODO summary:

  • Deprecate convert(Class, Class) style signatures.
  • Move null logic to convert(Object, Class) in NullConverter.
  • Add unit tests for primitive types, including "null" values
  • Test both ConvertService and individual Converters
  • Test expected canConvert == false scenarios as well, and not just the positive cases.

We would like to move away from these signatures as the process of
conversion implies there is an actual source Object to convert from,
which makes these signatures confusing. They should be unnecessary, and
thus removed in SJC 3.0.

This allows us to focus the functionality of the NullConverter, such
that its canConvert signatures always return true for a null Object, and
false for anything else.

This requires a temporary stop-gap of restoring some null class support
in the DefaultConverter, but only in deprecated methods.
Added a test for converting between primitive types.
Add a test class for individual converters. Currently only the
NullConverter is tested, as testing the DefaultConverter would
effectively be replicating all of the ConvertService test.
@hinerm
Copy link
Member Author

hinerm commented Sep 26, 2014

@ctrueden @dscho @dietzc I added the tests that made sense to me, and I think we are at at a minimal amount of work for implementing new Converters.

I think this is ready to merge but wanted to provide a final chance to object.

hinerm added a commit that referenced this pull request Sep 30, 2014
Adds more default implementations to the abstract Converter layer, simplifying the Converter implementation process. Adds a NullConverter to encapsulate null translation logic.
@hinerm hinerm merged commit 8e11e2e into master Sep 30, 2014
@ctrueden ctrueden deleted the convert-refining branch January 21, 2015 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants