[ 
https://issues.apache.org/jira/browse/SLING-2779?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13748499#comment-13748499
 ] 

Felix Meschberger commented on SLING-2779:
------------------------------------------

Some comments on the provided patch (dated Aug. 20th):

* If the DefaultsValueMap composites two maps, I wonder why it is a ValueMap... 
Particularly since this implementation does not properly support type 
conversions and expectations as defined for ValueMap
* The JavaDoc on the constructors are wrong and not very understandable
* The get(String) method does not really convert types thus, the get(String, 
Class<?>) method might in fact throw a ClassCastException which is not 
intended. I would assume that get(String) would actually turn to get(String, 
Class<?>) with an appropriate type instead of the other way around. Likewise 
for get(String, T)
* The ?: expressions are much complicated (and not enough parenthesized) to 
understand. Please change to explicit if-then-else. Otherwise errors may creep 
in which are hard to find
* The keySet(), entrySet(), and values() methods don't work: They either refer 
to the wrong defaults entries (e.g. values()) or modify the defaults map (e.g. 
keySet() gets the keySet() from the defaults and just adds to that set. This 
may or may not work and modify the defaults map, actually !
* The checkKey() method seems duplicate with existing ValueMap implementations

As a whole, this class looks rather fragile and I would not add this as is to 
our code base.
                
> Support for default properties values of a resource
> ---------------------------------------------------
>
>                 Key: SLING-2779
>                 URL: https://issues.apache.org/jira/browse/SLING-2779
>             Project: Sling
>          Issue Type: New Feature
>          Components: API
>    Affects Versions: API 2.3.0
>            Reporter: Gilles Knobloch
>         Attachments: SLING-2779.patch
>
>
> I already noticed several times it would be useful to be able to specify a 
> default properties for a resource:
> * if the resource itself contains the property, it will override the default 
> one.
> * but if it doesn't, the default value is used.
> This could be done either via:
> * specifying a {{sling:defaults}} property on the resource, which contains 
> the path to the resource which properties will be used by default.
> * providing a default map of properties
> Attaching a patch for review.
> For testing purpose, I put it under {{org.apache.sling.defaults}}, but I 
> imagine it could go to {{org.apache.sling.api.resource}}.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

Reply via email to