Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement map based properties per issue #41. #52

Closed
wants to merge 1 commit into from
Closed

Implement map based properties per issue #41. #52

wants to merge 1 commit into from

Conversation

NiXXeD
Copy link
Contributor

@NiXXeD NiXXeD commented Sep 23, 2013

Regarding:
#41

Implemented a first version of map based properties. This version supports Map<String, String> currently. Implementing different value types might be much more interesting, considering you might get type conversion errors per-entry.

Sample properties:

map1.value1=abc
map1.value2=def

Sample class:

interface MyConfig extends Config {
    @MapValue("map1")
    Map<String, String> map1();
}

@lviggiano
Copy link
Collaborator

Thanks @NiXXeD I'll review this and merge it asap.

@lviggiano
Copy link
Collaborator

I think it's a good starting point, but there are some catches. For instance the @MapValue annotation can possibly be removed and work as per usual properties.
Also other features like variable expansion and formatting probably do not work with the fields of the map.
Probably Map implementation needs to be more "in-deep" integrated with everything else. It's a complex thing (what about maps of maps, or maps of object? I don't know yet if they can be easily supported)

BTW, this is a great starting point.

@NiXXeD
Copy link
Contributor Author

NiXXeD commented Sep 25, 2013

When I was trying to do it without an annotation, I noticed that you'd (ideally) create the map at load time once so that multiple retrievals later would not have additional cost (perhaps for large configurations?). If there is an easy way to identify which properties would be a map during the create call and also what the prefix would be (without annotations), I can make that change and resubmit.

Regarding the generics parts, I wasn't sure where to go with that. You're right there is a ton of potential there for nesting and other types.

Regarding variable expansion, is that something that you perform at load or run time? I have to check again. If it's at load time, we certainly could do that with this implementation as well, but at run time you'd have to iterate the map again.

@lviggiano
Copy link
Collaborator

When I was trying to do it without an annotation, I noticed that you'd (ideally) create the map at load time once so that multiple retrievals later would not have additional cost (perhaps for large configurations?). If there is an easy way to identify which properties would be a map during the create call and also what the prefix would be (without annotations), I can make that change and resubmit.

Currently OWNER already identifies property keys. The only difference is that for Map the key is partial (the start of the key). So I believe that the @MapValue can be avoided (the method name or the @Key would do).

But this needs a deeper analysis, to handle things at best.

I don't want to provide an implementation which is non-optimal, since then after I would need to change it, or it can impose more limitations. Every new feature in OWNER that I am trying to add, should be comprehensive and fully integrated with everything that is already available.

Regarding the generics parts, I wasn't sure where to go with that. You're right there is a ton of potential there for nesting and other types.

Yeah... I guess nesting is something that needs to be kept apart at the moment.

Regarding variable expansion, is that something that you perform at load or run time? I have to check again. If it's at load time, we certainly could do that with this implementation as well, but at run time you'd have to iterate the map again.

It's done runtime.

A trick could be that the returned java.util.Map, can be backed by owner's internal properties data structure and work fine with expansion and other things (including conversion, so one could have Map<String, Integer> or Map<String,AnythingThatIsAlreadySupportedByOwnerConverters>).

Another thing that shouldn't be hard to support, if the Map is an implementation backed by Owner itself, are the paramerters:

interface MyConfig extends Config {
    Map<String, String> someMap(String someParam);
}

someParam would apply to all values in the Map.
So, I would say that if I need to implement this, I would make a Map implementation that keeps references to objects in owner that are capable to resolve expansion, formatting and conversion.
This is just an idea that is coming while I am writing now. But for sure, things are more complex and there are many things that need to be thought about (and possibly much code to be refactored to address this need).

I also think that Map support can be related to indexed properties that some users have requested (#48). It's a similar scenario:

map.keyA=valueOfA
map.keyB=valueOfB
...

list.1=first
list.2=second
...

So this adds more complexity, but when implementing the map thing, one should also think to an intelligent mechanism that allows OWNER to understand this special cases (since Lists/Set/Arrays are already supported in owner, and must be kept backward compatible, possibly)

The discussion may continue... since I feel this is just the tip of the iceberg.

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.

2 participants