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_ >
