Refactor on #388 PR #392

Emily-Jiang · 2018-12-09T23:44:56Z

No description provided.

…ojects. All ALv2, authors noted in source and NOTICE files.

All changes originally by me, except 1 smallish fixes by Tomas Langer.

This also includes a fix to avoid OS specific hacks (envconfig.* properties)

Signed-off-by: Emily Jiang <[email protected]>

jmesnil · 2018-12-11T10:45:05Z

tck/src/main/java/org/eclipse/microprofile/config/tck/ConfigAccessorTest.java

+            .addAsServiceProvider(ConfigSource.class, ConfigurableConfigSource.class)
+            .as(JavaArchive.class);
+
+        AbstractTest.addFile(testJar, "META-INF/javaconfig.properties");


@Emily-Jiang please change it to META-INF/microprofile-config.properties to make the test deploy properly

fixed. thanks @jmesnil

jmesnil · 2018-12-11T16:39:25Z

tck/src/main/java/org/eclipse/microprofile/config/tck/ConfigAccessorTest.java

+        Assert.assertEquals(config.access("tck.config.variable.secondEndpoint", String.class).build().getValue(),
+                "http://some.host.name/endpointTwo");
+
+        // variables in Config.getValue and getOptionalValue do not get evaluated otoh


why are they not evaluated? for backwards compatibility?

Does it support nested variable replacement? e.g. ${foo.${bar}}
Does it support default value? e.g. ${foo:123} -> returns 123 if the prop foo is not found
Does it support escaping? (so that the string ${ can appear without being an expression

They are not evaluated for backward compatibility. The variables were evaluated only via configaccess object.
${foo.${bar}} should be evaluated recursively.
At the moment, it does not support default value if a particular variable does not exist. I suggest For the case of ${foo}, if foo does not exist, we could either fail with exception or not to evaluate the variable.
Good point of the escaping aspect, we have not talked about this. I think it is a good idea to introduce escaping... how about ${... escape a escape character will be \${-> ${, ${ will be still escaped
@struberg what do you think?

Hi, I'm an outsider here but just wanted to comment that it would be nice if there was a way to be able to use variable expansion directly on the Config interface. Couldn't this be something set on org.eclipse.microprofile.config.spi.ConfigBuilder?

… - address comments Signed-off-by: Emily Jiang <[email protected]>

dmlloyd · 2018-12-17T15:27:14Z

api/src/main/java/org/eclipse/microprofile/config/ConfigAccessor.java

+
+    /**
+     * Returns the actual key which led to successful resolution and corresponds to the resolved value.
+     * This is useful when {@link ConfigAccessorBuilder#addLookupSuffix(String)} is used.


Unconstrained suffixes are not a good idea as they add a grammatical ambiguity that can lead to unexpected behavior, especially with large or complex configurations. How about adding some constraining rules instead? E.g. a syntax that is only valid for this case? Also, I'm not sure the suffix is the right affix to use; is there some established practice in this area? I know Play 1 used prefixes, and used % to disambiguate the case.

For reference, David is talking about https://www.playframework.com/documentation/1.5.x/ids

However, it seems that Play 2 has dropped out this feature and uses instead a simpler devSettings:
https://www.playframework.com/documentation/2.6.x/ConfigFile#Extra-devSettings

I'll remove this method for now and raise a separate issue for further discussion.

… - address comments Signed-off-by: Emily Jiang <[email protected]>

tevans78 · 2018-12-19T16:39:04Z

api/src/main/java/org/eclipse/microprofile/config/ConfigAccessorBuilder.java

+     * @param timeUnit the ChronoUnit for the value
+     * @return This builder
+     */
+    ConfigAccessorBuilder<T> cacheFor(long value, ChronoUnit timeUnit);


Should use Duration rather than a long and a ChronoUnit

will update to Duration.

tevans78 · 2018-12-20T14:28:34Z

api/src/main/java/org/eclipse/microprofile/config/spi/ConfigSource.java

+     * @return ChangeSupport informing the {@link org.eclipse.microprofile.config.Config} implementation about support for changes by this source
+     * @see ChangeSupport
+     */
+    default ChangeSupport onAttributeChange(Consumer<Set<String>> callback) {


Since a ConfigSource can be shared between multiple Config instances, this method should be an "add" and there should also be a corresponding "remove".

add two more methods:
RegisterChangeListener
ReleaseChangeListener

tevans78 · 2019-01-10T18:34:34Z

api/src/main/java/org/eclipse/microprofile/config/inject/ConfigProperty.java

@@ -135,4 +137,25 @@
     */
    @Nonbinding
    String defaultValue() default UNCONFIGURED_VALUE;
+


I don't know why this diff isn't flagging it up as a change but there should be a typo in the value of UNCONFIGURED_VALUE, an extra 'd'...

String UNCONFIGURED_VALUE="org.eclipse.microprofile.config.configproperty.unconfigureddvalue";

This was previously removed but then re-instated by PR #391

revert back to the master branch.

Emily-Jiang · 2019-01-18T13:11:20Z

api/src/main/java/org/eclipse/microprofile/config/ConfigAccessor.java

+
+    /**
+     * Returns the actual key which led to successful resolution and corresponds to the resolved value.
+     * This is useful when {@link ConfigAccessorBuilder#addLookupSuffix(String)} is used.


I'll remove this method for now and raise a separate issue for further discussion.

Emily-Jiang · 2019-01-18T13:12:18Z

api/src/main/java/org/eclipse/microprofile/config/ConfigAccessorBuilder.java

+     * @param timeUnit the ChronoUnit for the value
+     * @return This builder
+     */
+    ConfigAccessorBuilder<T> cacheFor(long value, ChronoUnit timeUnit);


will update to Duration.

Emily-Jiang · 2019-01-18T13:12:42Z

api/src/main/java/org/eclipse/microprofile/config/ConfigAccessorBuilder.java

+     * @param suffixValue fixed String to be used as suffix
+     * @return This builder
+     */
+    ConfigAccessorBuilder<T> addLookupSuffix(String suffixValue);


Will remove this method for now.

Emily-Jiang · 2019-01-18T13:14:05Z

api/src/main/java/org/eclipse/microprofile/config/inject/ConfigProperty.java

@@ -135,4 +137,25 @@
     */
    @Nonbinding
    String defaultValue() default UNCONFIGURED_VALUE;
+


revert back to the master branch.

Emily-Jiang · 2019-01-18T13:21:40Z

api/src/main/java/org/eclipse/microprofile/config/spi/ConfigSource.java

+     * @return ChangeSupport informing the {@link org.eclipse.microprofile.config.Config} implementation about support for changes by this source
+     * @see ChangeSupport
+     */
+    default ChangeSupport onAttributeChange(Consumer<Set<String>> callback) {


add two more methods:
RegisterChangeListener
ReleaseChangeListener

Signed-off-by: Emily Jiang <[email protected]>

Emily-Jiang · 2019-01-23T16:22:32Z

@jmesnil @OndroMih @tevans78 I have done the changes based on our discussion last week. please review the changes again. I would like to merge them before tomorrow's meeting, so that we can discuss how best to do prefix or suffix on property names.

Signed-off-by: Emily Jiang <[email protected]>

jmesnil

I'm ok with merging the PR as it stands (with further PRs to improve it)

struberg and others added 5 commits September 4, 2018 19:58

fix wording.

0dccb68

This feature got jointly developed in the DeltaSpike and ConfigJSR pr…

a0ae595

…ojects. All ALv2, authors noted in source and NOTICE files.

refs: #384 backport spec wording fixes and enhancements from ConfigJSR

442525f

All changes originally by me, except 1 smallish fixes by Tomas Langer.

refs: #384 backport tck for ConfigAccessor and fix TCK setup

a6717b5

This also includes a fix to avoid OS specific hacks (envconfig.* properties)

refactor #384 to update api and only track the dynamic config updates

bd2b3c9

Signed-off-by: Emily Jiang <[email protected]>

jmesnil reviewed Dec 11, 2018

View reviewed changes

refactor #384 to update api and only track the dynamic config updates…

47a3f47

… - address comments Signed-off-by: Emily Jiang <[email protected]>

jmesnil mentioned this pull request Dec 17, 2018

Add ability to decorate ConfigSources smallrye/smallrye-config#66

Closed

dmlloyd reviewed Dec 17, 2018

View reviewed changes

refactor #384 to update api and only track the dynamic config updates…

7907ec8

… - address comments Signed-off-by: Emily Jiang <[email protected]>

tevans78 reviewed Dec 19, 2018

View reviewed changes

tevans78 reviewed Dec 20, 2018

View reviewed changes

tevans78 reviewed Jan 10, 2019

View reviewed changes

Emily-Jiang mentioned this pull request Jan 14, 2019

Should Config.getValue() to support variable replacement #396

Closed

Emily-Jiang commented Jan 18, 2019

View reviewed changes

#384 address review comments

0e6f308

Signed-off-by: Emily Jiang <[email protected]>

#384 update copyright year

0593583

Signed-off-by: Emily Jiang <[email protected]>

jmesnil approved these changes Feb 8, 2019

View reviewed changes

Emily-Jiang merged commit e4b258b into master Feb 8, 2019

OndroMih mentioned this pull request Feb 8, 2019

Refactor #384 #398

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor on #388 PR #392

Refactor on #388 PR #392

Emily-Jiang commented Dec 9, 2018

jmesnil Dec 11, 2018

Emily-Jiang Dec 17, 2018

jmesnil Dec 11, 2018

Emily-Jiang Dec 17, 2018 •

edited

Loading

dmlloyd Dec 17, 2018

dmlloyd Dec 17, 2018 •

edited

Loading

jmesnil Jan 10, 2019

Emily-Jiang Jan 18, 2019

tevans78 Dec 19, 2018

Emily-Jiang Jan 18, 2019

tevans78 Dec 20, 2018

Emily-Jiang Jan 18, 2019 •

edited

Loading

tevans78 Jan 10, 2019

Emily-Jiang Jan 18, 2019

Emily-Jiang Jan 18, 2019

Emily-Jiang Jan 18, 2019

Emily-Jiang Jan 18, 2019

Emily-Jiang Jan 18, 2019

Emily-Jiang Jan 18, 2019 •

edited

Loading

Emily-Jiang commented Jan 23, 2019

jmesnil left a comment

Refactor on #388 PR #392

Refactor on #388 PR #392

Conversation

Emily-Jiang commented Dec 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emily-Jiang Dec 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmlloyd Dec 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emily-Jiang Jan 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emily-Jiang Jan 18, 2019 • edited Loading

Choose a reason for hiding this comment

Emily-Jiang commented Jan 23, 2019

jmesnil left a comment

Choose a reason for hiding this comment

Emily-Jiang Dec 17, 2018 •

edited

Loading

dmlloyd Dec 17, 2018 •

edited

Loading

Emily-Jiang Jan 18, 2019 •

edited

Loading

Emily-Jiang Jan 18, 2019 •

edited

Loading