On Mon, Dec 25, 2017 at 2:10 PM Mark Struberg <[email protected]> wrote:
> Hi John, Merry Christmas to you as well! > > > However, I only see support for implicit array converters > > Yes, that part was clarified by Emily on the ConfigJSR hangout. > She intentionally intended the functionality to be a Converter, not baked > in in ConfigImpl. > That way someone could also provide a custom Converter for Dog[], which > would then take precedence. > > I'm not sure myself whether this is such an important use case. > For normal situations it makes no difference, but if one registers a > custom coverter for an array type, then it is. > Btw, your array handling impl seems to be much smaller than mine. Guess we > could improve my version a bit - happy if you could take a look at it ;) > I've gone ahead and committed my changes into trunk. Take a look when you get a chance. It deals with the two issues I think I saw from reading your notes: - Array converters are meant to be implicit. IF a user registers Foo[].class then we must use that as the converter. I want to go back and test this as well (since I just blindly did it and confirmed no TCK failures :-) - Everything internally uses a MicroProfileTypedConverter for its work (which is nothing more than a consistent wrapper for MP and non MP converters, really needs a better name). BTW, did my question to the JSR EG come through? I'm specifically concerned about the case where a user wants to register List or Set of these types and there's no clear precedence. I think the approach currently taken is fine, where first we check if there is a Foo[].class, otherwise use Foo.class's converter when its an array type. However, if its a Set or LIst I'm only considering the Foo.class converter and not using the array converter if it was registered. I haven't compared but I suspect the ConfigJSR version is using the array converter if its registered. I think the only reason mine is shorter is because the work is split between creating the array list then the array. > > > but I think you should look at the approach I took on the escape > character handling in MP version. > > I do end up looping around the value twice (regex + foreach) > > I think the regexp version is much shorter in code and thus probably > easier to understand. > Otoh the manual char loop is much faster at execution time. It doesn't > need any regexp (performance intense) and only requires to loop over the > string once (due to a lookahead logic). > Both should deliver the same results though. > > > > One big difference I see, in my impl when you want a array, set or list, > I go string -> list first, then convert to either array or set if need be. > In the JSR case, I see it is doing string -> list -> array then to list/set > if need be > > > Yes, I tried to avoid auto-boxing if possible. Not sure I 100% succeeded > though and whether it really makes sense. A review of this part would > certainly be a good idea. I just wanted to make the TCK pass. > > > Perhaps we can blend the two together? > > +1 Would be much easier to maintain the two versions if we manage to keep > them in sync! > So on that note, I think we should start trying to figure out how to support both ConfigJSR and MP in a single codebase. They're similar enough that its feasible, I'm wondering if it makes sense to introduce unique builders per, but have a common underlying Config object in play, perhaps with projections for each API? > > txs and LieGrue, > strub > > > Am 25.12.2017 um 15:51 schrieb John D. Ament <[email protected]>: > > > > Hi & Merry Christmas guys! > > > > I believe you applied implicit converters to both branches. However, I > only see support for implicit array converters (maybe that's how you aimed > to do array types?). The actual implementation is pretty much the same, I > would just move the array logic into its own converter for consistency; but > I think you should look at the approach I took on the escape character > handling in MP version. I do end up looping around the value twice (regex > + foreach) > > > > One big difference I see, in my impl when you want a array, set or list, > I go string -> list first, then convert to either array or set if need be. > In the JSR case, I see it is doing string -> list -> array then to list/set > if need be. > > > > Perhaps we can blend the two together? > > > > John > > > > On Mon, Dec 25, 2017 at 5:24 AM Mark Struberg <[email protected]> wrote: > > Hi! > > > > +1 in general > > > > @John, please also take a look at the ConfigJSR branch. > > It already contains many of the functionality already as Emily just > copied over my changes from JSR-382 to mp-config. > > > > E.g all the implicit converters and array stuff is already done. > > Happy to have a hangout and do a pair review of it and then unify it > again. > > > > LieGrue, > > strub > > > > > Am 24.12.2017 um 08:23 schrieb Romain Manni-Bucau < > [email protected]>: > > > > > > Sounds good. Thanks for the heads up > > > > > > Le 24 déc. 2017 05:02, "John D. Ament" <[email protected]> a > écrit : > > > Hey all > > > > > > I just pushed up the last of the changes in the Geronimo config trunk > that would align to the MP Config 1.2 spec. There's still a bit of clean > up I want to aim for, so maybe after the new year we can plan to cut a > release of Config 1.1? > > > > > > You'll note in my commit, I moved most of the logic around converter > registration from ConfigImpl into DefaultConfigBuilder. I think this works > out a bit better, this way we only have to register converters that are > prioritized. I want to align Implicit converters into the > MicroProfileTypedConverter class, to clean up the code a bit more. > > > > > > John > > > >
