implement MockContentFragment_ContentElement_Structured getValue() me… #26
Conversation
|
thanks for the PR! it currently does not compile against AEM 6.5.7 API, which is the oldest version we support. however, this should not be a problem to be compatible, you have to remove some please also add unit tests for the new feature/methods. cosmetic: code needs to be reformatted to 2 spaces indendation and file headers, but i can add that later as well. |
|
removed the
|
|
thanks - i've done some cosmetic changes to reformat the code following our coding standards, and to reduce the visibility of the new classes as they are only interface implementations and should not be used directly. |
|
|
||
| @Override | ||
| public <T> @Nullable T getValue(Class<T> type) { | ||
| if (type.isInstance(value)) { |
There was a problem hiding this comment.
i think this is too simplified. according to https://developer.adobe.com/experience-manager/reference-materials/cloud-service/javadoc/com/adobe/cq/dam/cfm/FragmentData.html#getValue(java.lang.Class) this should support any conversion. what about using OSGi converter e.g.
public <T> @Nullable T getValue(Class<T> type) {
return Converters.standardConverter().convert(value).defaultValue(null).targetAs(type);
}
probably also update the unit tests a bit to test one or two actual conversions.
|
|
||
| @Override | ||
| public boolean isTypeSupported(Class type) { | ||
| return type.isInstance(value); |
There was a problem hiding this comment.
i think this is too simplfied as well? following https://developer.adobe.com/experience-manager/reference-materials/cloud-service/javadoc/com/adobe/cq/dam/cfm/FragmentData.html#isTypeSupported(java.lang.Class) it should return true for any type supported to set as value (supported by the internal converters). it's not related to the value currently set. for sake of simplicity we could just return true here with a comment that this is simplified.
|
@koenkicken We could use this as well, want to update this PR? |
Don't have time for this at the moment, so go ahead :) |
fixes #15