Geronimo Config 1.1?

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Geronimo Config 1.1?

John D. Ament
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
Reply | Threaded
Open this post in threaded view
|

Re: Geronimo Config 1.1?

Romain Manni-Bucau
Sounds good. Thanks for the heads up

Le 24 déc. 2017 05:02, "John D. Ament" <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Geronimo Config 1.1?

Mark Struberg
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 <[hidden email]>:
>
> Sounds good. Thanks for the heads up
>
> Le 24 déc. 2017 05:02, "John D. Ament" <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Geronimo Config 1.1?

John D. Ament
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 <[hidden email]> 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 <[hidden email]>:
>
> Sounds good. Thanks for the heads up
>
> Le 24 déc. 2017 05:02, "John D. Ament" <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Geronimo Config 1.1?

Mark Struberg
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 ;)

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

txs and LieGrue,
strub

> Am 25.12.2017 um 15:51 schrieb John D. Ament <[hidden email]>:
>
> 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 <[hidden email]> 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 <[hidden email]>:
> >
> > Sounds good. Thanks for the heads up
> >
> > Le 24 déc. 2017 05:02, "John D. Ament" <[hidden email]> 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
>

Reply | Threaded
Open this post in threaded view
|

Re: Geronimo Config 1.1?

John D. Ament


On Mon, Dec 25, 2017 at 2:10 PM Mark Struberg <[hidden email]> 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 <[hidden email]>:
>
> 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 <[hidden email]> 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 <[hidden email]>:
> >
> > Sounds good. Thanks for the heads up
> >
> > Le 24 déc. 2017 05:02, "John D. Ament" <[hidden email]> 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
>