Ok, forget about mpconfig1.4rc2 for one sec - we dont want to loose the
feature but we want to go one step further.
Think about this test - standalone, no need of arquillian or cdi:
1. Create a config instance - just for the test method - with 2
configsources: FIRST and LAST. Their name being representative of their
inversed order/ordinal.
2. FIRST has the key a=b
3. LAST has the key a=o.a.g.c.null (the new constant)
config.getOptionalValue("a") must be empty since LAST will override FIRST
value.
A concrete example is microprofile-config.properties having a default and a
system properties resetting it to null.
Once this test pass, let's handle the injections. What does change? You
dont want to use config.get[Optional]Value anymore but you want a
ConfigImpl which returns if a value was found, even if it is null.
So we must add ConfigImpl#getIntervalValue(...) which returns ValueResult
{boolean found, object value [, ConfigSource from]} and use that instead of
getValue or getOptionalValue in the bean.
Here, no more null check - the constant can be private ;) - since the found
boolean replaces it.
Hope it makes sense.
Le mer. 5 févr. 2020 à 01:20, Daniel Cunha <[email protected]> a écrit :
> Hey Romain,
>
> I updated the PR, but not sure if I get how you are thinking about it.
>
> Check it and if not ok, follow up with it.
>
> --
> Daniel "soro" Cunha
> https://twitter.com/dvlc_
>
> On Tue, Feb 4, 2020, 08:23 Daniel Cunha <[email protected]> wrote:
>
>> Hi Romain,
>>
>> yeah.. I can handle that.
>>
>> Thank you!
>>
>> Em ter., 4 de fev. de 2020 às 02:48, Romain Manni-Bucau <
>> [email protected]> escreveu:
>>
>>> Hi Daniel,
>>>
>>> Thanks for the update.
>>> This is an iso-change, thought we were moving null handling to
>>> ConfigImpl rather than ConfigInjectionBean to enable it in
>>> Config.get[Optional]Value too (just requires an interval getRawValue for
>>> injection but nothing crazy), this is why i spoke about the constant in
>>> ConfigImpl and didnt expect Defaults class.
>>> Do you want to handle the ConfigImpl change? If not i can follow up.
>>>
>>> Le mar. 4 févr. 2020 à 02:58, Daniel Cunha <[email protected]> a
>>> écrit :
>>>
>>>> I had updated the PR:
>>>> https://github.com/apache/geronimo-config/pull/7/files
>>>> Hope it sounds good now.
>>>>
>>>> Em sáb., 1 de fev. de 2020 às 08:25, Daniel Cunha <[email protected]>
>>>> escreveu:
>>>>
>>>>> Hmm..
>>>>>
>>>>> Good catch! So, +1 to proceed with your strategy.
>>>>>
>>>>> Em sáb., 1 de fev. de 2020 às 07:39, Romain Manni-Bucau <
>>>>> [email protected]> escreveu:
>>>>>
>>>>>> Not really, half of the cases will be in databases or properties file
>>>>>> so the constant does not help.
>>>>>> Other half is in config property but if you use a spec you dont want
>>>>>> to depend directly on the impl so better to not expose it yet IMHO (it
>>>>>> will
>>>>>> be fixed in the spec in 2.0 anyway).
>>>>>> That said ill not fight against a constant in ConfigImpl.
>>>>>>
>>>>>> Le sam. 1 févr. 2020 à 11:04, Daniel Cunha <[email protected]> a
>>>>>> écrit :
>>>>>>
>>>>>>> Ok, I get it.
>>>>>>>
>>>>>>> But the user should define that value in the ConfigProperty. I mean
>>>>>>> that with the constant it keep more fancy/easy/safe to the user. The
>>>>>>> constant is just a simplify way to not need to memorize the property
>>>>>>> like:
>>>>>>> org.apache.geronimo.config.value.NULL.
>>>>>>> It's better to use Constants.NULL_VALUE instead of set
>>>>>>> org.apache.geronimo.config.value.NULL in the ConfigProperty. IMHO.
>>>>>>>
>>>>>>> Em sáb., 1 de fev. de 2020 às 06:38, Romain Manni-Bucau <
>>>>>>> [email protected]> escreveu:
>>>>>>>
>>>>>>>> The only location we need that constant is there:
>>>>>>>> https://github.com/apache/geronimo-config/blob/trunk/impl/src/main/java/org/apache/geronimo/config/ConfigImpl.java#L135
>>>>>>>>
>>>>>>>> Something like return
>>>>>>>> "org.apache.geronimo.config.value.NULL".equals(value) ? null : value;
>>>>>>>>
>>>>>>>> We also don't want the users to import
>>>>>>>> org.apache.geronimo.config.value.Constants in their code so not sure a
>>>>>>>> constant or enum is needed, was just that.
>>>>>>>>
>>>>>>>> Romain Manni-Bucau
>>>>>>>> @rmannibucau <https://twitter.com/rmannibucau> | Blog
>>>>>>>> <https://rmannibucau.metawerx.net/> | Old Blog
>>>>>>>> <http://rmannibucau.wordpress.com> | Github
>>>>>>>> <https://github.com/rmannibucau> | LinkedIn
>>>>>>>> <https://www.linkedin.com/in/rmannibucau> | Book
>>>>>>>> <https://www.packtpub.com/application-development/java-ee-8-high-performance>
>>>>>>>>
>>>>>>>>
>>>>>>>> Le sam. 1 févr. 2020 à 10:34, Daniel Cunha <[email protected]>
>>>>>>>> a écrit :
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Not sure if I get your point and applicability of it.
>>>>>>>>>
>>>>>>>>> Em sáb., 1 de fev. de 2020 às 04:49, Romain Manni-Bucau <
>>>>>>>>> [email protected]> escreveu:
>>>>>>>>>
>>>>>>>>>> Normally it should be used in a single place in ConfigImpl - dont
>>>>>>>>>> think we want users to see it, g-config should stay in scope runtime
>>>>>>>>>> in
>>>>>>>>>> projects - so hardcoding it is ok and simpler to read but not a big
>>>>>>>>>> deal if
>>>>>>>>>> it is a constant.
>>>>>>>>>>
>>>>>>>>>> Le sam. 1 févr. 2020 à 04:27, Daniel Cunha <[email protected]>
>>>>>>>>>> a écrit :
>>>>>>>>>>
>>>>>>>>>>> Hey,
>>>>>>>>>>>
>>>>>>>>>>> I saw your comment on my PR.
>>>>>>>>>>> So, I like the idea to keep it as you mentioned, maybe we can
>>>>>>>>>>> move it for an Interface with some constants or better an Enum.
>>>>>>>>>>> So we can keep the NULL_VALUES still on the game in Geronimo,
>>>>>>>>>>> since it was removed in 1.4-RC3. :)
>>>>>>>>>>>
>>>>>>>>>>> So, if the spec choose to continue with a strategy like
>>>>>>>>>>> NULL_VALUES we'll continue support spec and our implementation as
>>>>>>>>>>> well.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Em sex., 31 de jan. de 2020 às 23:18, Daniel Cunha <
>>>>>>>>>>> [email protected]> escreveu:
>>>>>>>>>>>
>>>>>>>>>>>> Sounds good. I'll update the PR for it. :)
>>>>>>>>>>>>
>>>>>>>>>>>> --
>>>>>>>>>>>> Daniel "soro" Cunha
>>>>>>>>>>>> https://twitter.com/dvlc_
>>>>>>>>>>>>
>>>>>>>>>>>> On Fri, Jan 31, 2020, 18:35 Romain Manni-Bucau <
>>>>>>>>>>>> [email protected]> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Guess we can just use a custom geronimo constant and keep the
>>>>>>>>>>>>> feature. It is needed in a lot of apps anyway and we dont need to
>>>>>>>>>>>>> break it
>>>>>>>>>>>>> in mpconfig 2 when the spec will have another solution.
>>>>>>>>>>>>>
>>>>>>>>>>>>> We should also wire it in Config to be able to reset a value
>>>>>>>>>>>>> (using source ordinals).
>>>>>>>>>>>>>
>>>>>>>>>>>>> Wdyt?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Le ven. 31 janv. 2020 à 22:26, Daniel Cunha <
>>>>>>>>>>>>> [email protected]> a écrit :
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi Folks,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Changes for MicroProfile Config 1.4-RC3. PR:
>>>>>>>>>>>>>> https://github.com/apache/geronimo-config/pull/7
>>>>>>>>>>>>>> The NULL_VALUE was reverted. TCK and our tests is passing as
>>>>>>>>>>>>>> expected. :)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Best regard
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Em dom., 26 de jan. de 2020 às 17:20, Mark Struberg <
>>>>>>>>>>>>>> [email protected]> escreveu:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> lgtm,
>>>>>>>>>>>>>>> Thanks Daniel and also Romain!
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> LieGrue,
>>>>>>>>>>>>>>> strub
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> > Am 26.01.2020 um 16:22 schrieb Romain Manni-Bucau <
>>>>>>>>>>>>>>> [email protected]>:
>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>> > FYI I just fixed master code - test was using the proxy
>>>>>>>>>>>>>>> fields instead of injected values. Feel free to review and
>>>>>>>>>>>>>>> enhance if
>>>>>>>>>>>>>>> needed.
>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>> > Romain Manni-Bucau
>>>>>>>>>>>>>>> > @rmannibucau | Blog | Old Blog | Github | LinkedIn | Book
>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>> > Le dim. 26 janv. 2020 à 08:28, Romain Manni-Bucau <
>>>>>>>>>>>>>>> [email protected]> a écrit :
>>>>>>>>>>>>>>> > Merged, thks a lot Daniel
>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>> > Le dim. 26 janv. 2020 à 01:25, Daniel Cunha <
>>>>>>>>>>>>>>> [email protected]> a écrit :
>>>>>>>>>>>>>>> > I believe now it's in a good shape.
>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>> > Thank you, Romain.
>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>> > --
>>>>>>>>>>>>>>> > Daniel "soro" Cunha
>>>>>>>>>>>>>>> > https://twitter.com/dvlc_
>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>> > On Sat, Jan 25, 2020, 17:55 Romain Manni-Bucau <
>>>>>>>>>>>>>>> [email protected]> wrote:
>>>>>>>>>>>>>>> > You dont need to parse constants :
>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>> > Long.parseLong("0") -> 0L ;)
>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>> > Otherwise looks perfect for me
>>>>>>>>>>>>>>> > If nobody shouts, i will merge it tmr or on monday
>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>> > Le sam. 25 janv. 2020 à 20:10, Daniel Cunha <
>>>>>>>>>>>>>>> [email protected]> a écrit :
>>>>>>>>>>>>>>> > Changes sent!
>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>> > Thank you for your review Romain.
>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>> > Em sáb., 25 de jan. de 2020 às 15:05, Romain Manni-Bucau <
>>>>>>>>>>>>>>> [email protected]> escreveu:
>>>>>>>>>>>>>>> > Proxy supports primitives so default is not always null
>>>>>>>>>>>>>>> compared to injections, no? Once this point materialized by a
>>>>>>>>>>>>>>> test - and
>>>>>>>>>>>>>>> maybe imports reorganized to minimize the diff? - i guess we
>>>>>>>>>>>>>>> are good to
>>>>>>>>>>>>>>> merge.
>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>> > Le sam. 25 janv. 2020 à 18:37, Daniel Cunha <
>>>>>>>>>>>>>>> [email protected]> a écrit :
>>>>>>>>>>>>>>> > I updated the PR. Hope it is in a good shape now!
>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>> > Thank you.
>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>> > Em sáb., 25 de jan. de 2020 às 13:08, Romain Manni-Bucau <
>>>>>>>>>>>>>>> [email protected]> escreveu:
>>>>>>>>>>>>>>> > Except a small import issue (*) i guess it just needs the
>>>>>>>>>>>>>>> proxy handling (in our invocation handler)of default value and
>>>>>>>>>>>>>>> some test(s)
>>>>>>>>>>>>>>> then it looks pretty good to me.
>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>> > Le sam. 25 janv. 2020 à 17:01, Daniel Cunha <
>>>>>>>>>>>>>>> [email protected]> a écrit :
>>>>>>>>>>>>>>> > Hi Folks,
>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>> > https://github.com/apache/geronimo-config/pull/6
>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>> > That is the PR with changes to cover MicroProfile 1.4-RC
>>>>>>>>>>>>>>> on Geronimo Config.
>>>>>>>>>>>>>>> > I really appreciate if someone could put the eyes on it.
>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>> > Thank you.
>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>> > --
>>>>>>>>>>>>>>> > Daniel "soro" Cunha
>>>>>>>>>>>>>>> > https://twitter.com/dvlc_
>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>> > --
>>>>>>>>>>>>>>> > Daniel "soro" Cunha
>>>>>>>>>>>>>>> > https://twitter.com/dvlc_
>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>> > --
>>>>>>>>>>>>>>> > Daniel "soro" Cunha
>>>>>>>>>>>>>>> > https://twitter.com/dvlc_
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> --
>>>>>>>>>>>>>> Daniel "soro" Cunha
>>>>>>>>>>>>>> https://twitter.com/dvlc_
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> --
>>>>>>>>>>> Daniel "soro" Cunha
>>>>>>>>>>> https://twitter.com/dvlc_
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> Daniel "soro" Cunha
>>>>>>>>> https://twitter.com/dvlc_
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Daniel "soro" Cunha
>>>>>>> https://twitter.com/dvlc_
>>>>>>>
>>>>>>
>>>>>
>>>>> --
>>>>> Daniel "soro" Cunha
>>>>> https://twitter.com/dvlc_
>>>>>
>>>>
>>>>
>>>> --
>>>> Daniel "soro" Cunha
>>>> https://twitter.com/dvlc_
>>>>
>>>
>>
>> --
>> Daniel "soro" Cunha
>> https://twitter.com/dvlc_
>>
>
