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

[DISCUSS] move Config#getConfigSources back to returning an Array or List<ConfigSource> ? #67

Closed
struberg opened this issue Feb 23, 2017 · 7 comments
Milestone

Comments

@struberg
Copy link
Contributor

Config#getConfigSources originally have been a ConfigSource[] [1,2]

This got moved to an Iterable.
But that way we might end up loosing a few use cases.

The ConfigSources are sorted in a descending way. Means the ones with the highest ordinal are in first position (btw we should define this more precisely). Sometimes you need exactly this ordering. E.g. if you want to know the value of a single key then you iterate from the ConfigSource with the highest ordinal and once you find a value you are done.

But think about e.g. an admin UI as @starksm64 mentioned. In that case you want to start at the ConfigSource with the lowest ordinal and drop all the values into a big Map<String, String>. Then you take the next one and again just drop all the values into the map. Effectively overwriting the lower values IF they got also specified in a higher ordinal ConfigSource.

But for doing that you have to iterate in an ASCENDING way! With just an Iterable you cannot easily do that without having to repackage it, right? So what do we win with the Iterable over a good old array?

LieGrue,
strub

[1] https://github.com/struberg/javaConfig/blob/master/api/src/main/java/io/microprofile/config/Config.java#L85
[2] https://github.com/apache/deltaspike/blob/master/deltaspike/core/api/src/main/java/org/apache/deltaspike/core/api/config/ConfigResolver.java#L476

@gunnarmorling
Copy link
Contributor

gunnarmorling commented Mar 3, 2017

Yes, I also stumbled upon the return type of Iterable. That's most of the time not useful for callers of a method.

I'd return a SortedSet: set as no source should be in there only once, and sorted so Mark's ordering requirement is satisfied.

@struberg
Copy link
Contributor Author

@gunnarmorling a SortedSet is semantically probably the most correct one. But that requires that the ConfigSource would need to implement the Comparator interface. So I think having a defined sort order and an unmodifyable List is probably the best

@struberg
Copy link
Contributor Author

Or simply an array. Makes it immutable automatically.

@gunnarmorling
Copy link
Contributor

gunnarmorling commented Mar 10, 2017 via email

@struberg
Copy link
Contributor Author

Indeed you are right, moving to SortedSet.

@struberg struberg reopened this Mar 10, 2017
@struberg
Copy link
Contributor Author

gnn SortedSet has no index access, you need to resort again. And it's mutable.

struberg added a commit that referenced this issue Mar 10, 2017
This reverts commit bf78fec.

Turns out Iterable is still a tad better than arrays as they are unmodifyable
Signed-off-by: Mark Struberg <[email protected]>
struberg added a commit that referenced this issue Mar 10, 2017
Signed-off-by: Mark Struberg <[email protected]>
@struberg struberg reopened this Mar 10, 2017
@struberg
Copy link
Contributor Author

sticking with Iterable after a discussion with Gunnar

struberg added a commit that referenced this issue Mar 10, 2017
@struberg struberg modified the milestone: config-1.0 Mar 10, 2017
struberg added a commit to struberg/microprofile-config that referenced this issue Mar 10, 2017
Signed-off-by: Mark Struberg <[email protected]>
OndroMih pushed a commit to OndroMih/microprofile-config that referenced this issue Mar 14, 2017
This is needed as most use cases need the ConfigSources in the effectively sorted order.
They also need it sometimes in descending and for other use cases in ascending ordinal order.
This cannot be achieved by an Iterable in easy ways.
Array has the implicit advantage that it is automatically immutable.

Signed-off-by: Mark Struberg <[email protected]>
OndroMih pushed a commit to OndroMih/microprofile-config that referenced this issue Mar 14, 2017
This reverts commit bf78fec.

Turns out Iterable is still a tad better than arrays as they are unmodifyable
Signed-off-by: Mark Struberg <[email protected]>
OndroMih pushed a commit to OndroMih/microprofile-config that referenced this issue Mar 14, 2017
Signed-off-by: Mark Struberg <[email protected]>
OndroMih pushed a commit to OndroMih/microprofile-config that referenced this issue Mar 14, 2017
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

No branches or pull requests

2 participants