Fixes #178

[kenfinnigan]

No description provided.

[dmlloyd]

Needs a serialVersionUID.

[kenfinnigan]

This morning I realized I messed up and we do need the parent separate. If we don’t, any config source artifact requires the full implementation, which negates the benefit of config sources being separate as they then can’t be used in other implementations

…

Sent from my iPhone

On Nov 8, 2019, at 16:37, David M. Lloyd ***@***.***> wrote:  @dmlloyd commented on this pull request. In implementation/src/main/java/io/smallrye/config/AbstractConfigSource.java: > @@ -0,0 +1,33 @@ +package io.smallrye.config; + +import java.io.Serializable; + +import org.eclipse.microprofile.config.spi.ConfigSource; + +public abstract class AbstractConfigSource implements ConfigSource, Serializable { + private int ordinal; Making ordinal mutable could cause trouble. It should be final. That does raise a challenge however for config sources which configure their ordinal from their own data (is that all of them?); it might be worth introducing an intermediate class or classes to support this (e.g. a MapBasedConfigSource which accepts a Map parameter could call super(name, Integer.parseInt(map.getOrDefault(...)); and the properties config source could call e.g. super(propertiesToMap(properties)) or some such). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[dmlloyd]

Ah I didn't realize that it was a goal to be able to use these config sources with other implementations. Even the default ones?

[dmlloyd]

TBH I'm not sure this is a worthwhile goal. Does it really make sense? What is an example use case?

[kenfinnigan]

It's just the parents that would need to be extracted actually. The default ones can still be inside the main impl.

It's a two-part goal. One is because that was the goal of MP Extensions, which we're accepting a donation from, and two it would help drive usage and adoption of SmallRye, possibly with a view to users/implementors switching to the full config impl at some point

[dmlloyd]

Okay so then I guess the strategy could be:

• Put the base classes and common utils in one module which only depends on the API
• Implementation contains the built-in default sources, depends on CS base module
• Sources modules depend only on CS base module

[kenfinnigan]

That's a good explanation of what I was thinking, thanks

[dmlloyd]

👍 Sorry for being dense!

[kenfinnigan]

To be fair, I wasn't really explaining what I was thinking.

So the clarification was good for both of us

[kenfinnigan]

Ok, think we're on the right track now

[kenfinnigan]

Thanks @dmlloyd. Do you think this warrants a bump to 1.4.x?

[dmlloyd]

Yes I think that's reasonable. There are now two artifacts to dependency-manage which is a pretty substantial change.