Hi Romain, +1.
I'll review it and procude what you are saying. I see now much better what you was trying explaining me. I'll send a update as fast as I can :) Thank you Em qua., 5 de fev. de 2020 às 02:53, Romain Manni-Bucau < [email protected]> escreveu: > 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_ >>> >> -- Daniel "soro" Cunha https://twitter.com/dvlc_
