Improve Javadoc etc

[Emily-Jiang]

No description provided.

[gunnarmorling]

Hey Emily, could you commit with your e-mail address you are using here on GitHub, too? That way the commits will be properly attributed to you in the contribution history.

[gunnarmorling]

Hi Emily, added some comments inline. Most notably, I don't see the value of ConfigValue at this point.

[gunnarmorling]

What's the rationale for changing it to not return Optional any more?

[OndroMih]

I assume that the method is intended for debugging rather than for business logic, therefore it is counter-productive to wrap the result in an Optional. It would make even sense to me to rename the method to something much more complicated to discourage using the method in normal business logic :) But I'm serious - far too many people would use it for normal code if its name stays like this.

[gunnarmorling]

it is counter-productive

Why is that? The value may be present or not, which is exactly the semantics of Optional. Optional was introduced to represent an optional return value; let's use it instead of requiring people to think about whether it can be null or not. Optional conveys that clearly.

[gunnarmorling]

That said I'm unsure what's the use case for this method in the first place? Should it actually be present here if we are concerned about it being used? Raw values can be obtained from config sources so we may leave it out here in the first round and see whether there's actual demand for it?

UPDATE: I'll create a separate issue to facilitate a discussion on that last comment.

[OndroMih]

IIRC, it is really meant for debugging purposes to see what is the final value over all config sources (even if multiple sources provide it), but before it goes into a converter. Since converters work with String, it's natural to return raw value as a String.

Maybe we should leave this method out for now, and only suggest that implementations provide vendor-specific means to access the final raw value, provided that it is only for debugging as I assume.

[gunnarmorling]

Since converters work with String, it's natural to return raw value as a String.

That's what I meant above, I doubt basing it on String is the right thing to do. I've opened #87 to discuss this separately.

[Emily-Jiang]

This method is useful for debugging purpose as commented by @OndrejM !

[OndroMih]

Maybe we could avoid the getRawString method in config-1.0 and rethink if we need to standardize this or leave it up to the implementations?

[keilw]

Optional as mentioned in recent calls is quite ugly especially when you try to combine it with type-safety you quickly stretch it beyond its limit.
Typesafe Config just to give one example has String getString(String path); and getters for most other base types including primitives. ConfigObject getObject(String path); returns a ConfigObject which is a specialized ConfigValue And Object getAnyRef(String path); is the "raw" equivalent simply returning the underlying value as Object. No Optional, although the library is now strongly tailored to Java SE 8, e.g. HOCON dealing with java.time values like Duration or LocalDate.

[gunnarmorling]

If these are all the methods on ConfigValue, I don't see good reason for adding it at this point. You'll get the same functionality (and easier to use) when directly injecting values into beans. We should introduce ConfigValue if it provides additional value.

[OndroMih]

I think that adding ConfigValue in this PR doesn't make sense at all. It should be either in a separate PR, or coupled with the CDI PR (#42), where I already added it. I think that for a start, it makes sense to have it even with 2 methods, we can add more methods later.

@Emily-Jiang I suggest that you create a PR with ConfigValue against my repo, so that I update the existing PR #42.

I'd discuss direct injection separately, I created PR #86 for this reason.

[Emily-Jiang]

@OndrejM I'll create this against your PR #42 and merge in.

[Emily-Jiang]

ConfigValue can do the injection for the custom class type T. You can do
@Inject @ConfigProperty("myPropName") ConfigValue propValue;

also the injection can cope with dynamic changes.

[gunnarmorling]

The import should go before the JavaDoc.

[OndroMih]

I would clarify that it is not possible to add a custom config source programmatically into the default config retrieved by ConfigProvider.getConfig(). It is only possible when building a completely new config instance.

[Emily-Jiang]

No, it is possible. The calling of ConfigProvider.getConfig() should trigger the builder to add default configure sources and autodiscovered configure sources. Otherwise, the ConfigProvider.getConfig is not useful. Thoughts? We should clarify this.

[OndroMih]

See, that's why I recommended added more test code to demonstrate the usage. This part is not very clear to me. How can I change what is returned by ConfigProvider.getConfig() programmatically? I mean, can I somehow hook into the builder used to build that config, without adding config sources via the service locator? Would this work: ConfigProvider.getBuilder().withSources(myCustomSource); Config configWithMyCustomSource = ConfigProvider.getConfig(); ?

[OndroMih]

For supporting @priority, we should include common annotations JSR (JSR-250) into MP, thoughts?

[gunnarmorling]

I see your point, but my experiences with 250 are very mixed.

There are several conflicting dependencies with (parts of) its types out there, some are even in the JDK itself. So it's very prone to cause classpath conflicts, i.e. users will end up with the same types being pulled in multiple times from several dependencies. Which is esp. an issue when thinking about Java 9 / Jigsaw. Hence I'd try to avoid that dependency as much as possible.

[OndroMih]

I agree, but I don't see how we get @Priority annotation into MicroProfile then. Which API artifact would bring it? In the current state, there's no way to build an app with @Priority with just MP dependencies.

[gunnarmorling]

The question is, does it have to be pulled in by the API itself? I don't think there is any dependency from the API requiring it. Instead it can be left to providers or the application using the mP.

[OndroMih]

@gunnarmorling The question is, does it have to be pulled in by the API itself?

I get it now. I was not sure if makes sense to instruct anyone to use @Priority if it is not provided by the container. But since it's an edge case and it is possible to provide the annotation within the application, I'm fine to leave the spec as is.

[Emily-Jiang]

Wondering why my sign off failed ip-validation check? Any suggestions?

[gunnarmorling]

You have commits with two different e-mails in this PR, one of which doesn't have the ECA on record. I think the best would be if you do your ECA with the e-mail address you have on GitHub (you also can change the email address in the ECA tool) and squash all commits into a single one, using that e-mail address. That way the ECA check should be happy and the commits will properly be attributed to your account in the GH history and stats.

[struberg]

+1, but please note that we should never squash history which contains commits of different persons as that would destroy code provenance. So please only squash your own commits.

[OndroMih]

What I do to pass eclipse IP checks is that I rebase my local branch interactively, and then re-commit each revision with the correct user (email) and sign-off. Then I force push my changes. Not ideal either, but the history is preserved. I don't know how that affects comments in the PR.

[Emily-Jiang]

thanks @gunnarmorling @struberg @OndrejM! Finally worked out! I ended up abandoned some commits and redo it under a different PR. I'm going to merge this javadoc changes.