You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tamaya.apache.org by peculater <gi...@git.apache.org> on 2018/02/01 20:23:19 UTC
[GitHub] incubator-tamaya pull request #11: TAMAYA-327: Consistent signature creating...
GitHub user peculater opened a pull request:
https://github.com/apache/incubator-tamaya/pull/11
TAMAYA-327: Consistent signature creating ConversionContexts
Comments in ConversionContext lead me to believe that supportedFormats
should have their order maintained after import. The HashSet that the
Builder object uses as a backer will not accomplish that goal. This
change converts the Builder to use ArrayLists for supportedFormats, like
the object proper.
Some additional tests have been added here in pursuit of TAMAYA-288's
additional code coverage.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/peculater/incubator-tamaya TAMAYA-327
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-tamaya/pull/11.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #11
----
commit f63ba99ffaf4050827fc0ad88cd5df91f506956d
Author: William Lieurance <wi...@...>
Date: 2018-02-01T20:16:27Z
TAMAYA-327: Consistent signature creating ConversionContexts
Comments in ConversionContext lead me to believe that supportedFormats
should have their order maintained after import. The HashSet that the
Builder object uses as a backer will not accomplish that goal. This
change converts the Builder to use ArrayLists for supportedFormats, like
the object proper.
Some additional tests have been added here in pursuit of TAMAYA-288's
additional code coverage.
----
---
[GitHub] incubator-tamaya issue #11: TAMAYA-327: Consistent signature creating Conver...
Posted by peculater <gi...@git.apache.org>.
Github user peculater commented on the issue:
https://github.com/apache/incubator-tamaya/pull/11
I've refactored this to use LinkedHashSets instead of ArrayLists in the 2 places I was worried about. I think that covers what was discussed on the mailing list?
---
[GitHub] incubator-tamaya pull request #11: TAMAYA-327: Consistent signature creating...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-tamaya/pull/11
---
[GitHub] incubator-tamaya pull request #11: TAMAYA-327: Consistent signature creating...
Posted by peculater <gi...@git.apache.org>.
Github user peculater commented on a diff in the pull request:
https://github.com/apache/incubator-tamaya/pull/11#discussion_r165699393
--- Diff: code/api/src/main/java/org/apache/tamaya/spi/ConversionContext.java ---
@@ -145,7 +145,7 @@ public ConfigurationContext getConfigurationContext() {
/** The injection target (only set with injection used). */
private AnnotatedElement annotatedElement;
/** The ordered list of formats tried. */
- private final Set<String> supportedFormats = new HashSet<>();
+ private final List<String> supportedFormats = new ArrayList<>();
--- End diff --
It would. We'd also want to change https://github.com/apache/incubator-tamaya/pull/11/files#diff-0ff94b3abf9ec69ee193d9ecdaad4936R42 to make it consistent.
---
Re: [GitHub] incubator-tamaya pull request #11: TAMAYA-327:
Consistent signature creating...
Posted by Anatole Tresch <at...@gmail.com>.
+1
The JSR actually will only support one single Converter instance per type,
whereas Tamaya supports multiple converters. Consequently there is no
constraint regarding converter ordering.
Additionally the fix make definitely sense for me from a usage perspective.
2018-02-04 20:10 GMT+01:00 P. Ottlinger <po...@apache.org>:
> Hi,
>
> interesting bug you spotted :-)
>
> @Anatole: does the configJSR say anything about an order?
>
> To my mind keeping the order (FIFO) seems logically but I wouldn't want
> to break compatibility with the coming JSR ....
>
> Apart from that: +1 since it's more natural and maybe we should enhance
> the javadoc as well to make it more clear for consumers.
>
> Just my 2ct
> Phil
>
> Am 01.02.2018 um 21:23 schrieb peculater:
> > GitHub user peculater opened a pull request:
> >
> > https://github.com/apache/incubator-tamaya/pull/11
> >
> > TAMAYA-327: Consistent signature creating ConversionContexts
> >
> > Comments in ConversionContext lead me to believe that
> supportedFormats
> > should have their order maintained after import. The HashSet that
> the
> > Builder object uses as a backer will not accomplish that goal. This
> > change converts the Builder to use ArrayLists for supportedFormats,
> like
> > the object proper.
> >
> > Some additional tests have been added here in pursuit of TAMAYA-288's
> > additional code coverage.
> >
> > You can merge this pull request into a Git repository by running:
> >
> > $ git pull https://github.com/peculater/incubator-tamaya TAMAYA-327
> >
> > Alternatively you can review and apply these changes as the patch at:
> >
> > https://github.com/apache/incubator-tamaya/pull/11.patch
> >
> > To close this pull request, make a commit to your master/trunk branch
> > with (at least) the following in the commit message:
> >
> > This closes #11
> >
> > ----
> > commit f63ba99ffaf4050827fc0ad88cd5df91f506956d
> > Author: William Lieurance <wi...@...>
> > Date: 2018-02-01T20:16:27Z
> >
> > TAMAYA-327: Consistent signature creating ConversionContexts
> >
> > Comments in ConversionContext lead me to believe that
> supportedFormats
> > should have their order maintained after import. The HashSet that
> the
> > Builder object uses as a backer will not accomplish that goal. This
> > change converts the Builder to use ArrayLists for supportedFormats,
> like
> > the object proper.
> >
> > Some additional tests have been added here in pursuit of TAMAYA-288's
> > additional code coverage.
> >
> > ----
> >
> >
> > ---
> >
>
>
--
*Anatole Tresch*
PPMC Member Apache Tamaya
JCP Star Spec Lead
*Switzerland, Europe Zurich, GMT+1*
*maketechsimple.wordpress.com <http://maketechsimple.wordpress.com/> *
*Twitter: @atsticks, @tamayaconf*
*Speaking at:*
[image: JSD_Speaker_2017][image: J-Con 2017 logo][image: JVM Con]
Re: [GitHub] incubator-tamaya pull request #11: TAMAYA-327:
Consistent signature creating...
Posted by "P. Ottlinger" <po...@apache.org>.
Hi,
interesting bug you spotted :-)
@Anatole: does the configJSR say anything about an order?
To my mind keeping the order (FIFO) seems logically but I wouldn't want
to break compatibility with the coming JSR ....
Apart from that: +1 since it's more natural and maybe we should enhance
the javadoc as well to make it more clear for consumers.
Just my 2ct
Phil
Am 01.02.2018 um 21:23 schrieb peculater:
> GitHub user peculater opened a pull request:
>
> https://github.com/apache/incubator-tamaya/pull/11
>
> TAMAYA-327: Consistent signature creating ConversionContexts
>
> Comments in ConversionContext lead me to believe that supportedFormats
> should have their order maintained after import. The HashSet that the
> Builder object uses as a backer will not accomplish that goal. This
> change converts the Builder to use ArrayLists for supportedFormats, like
> the object proper.
>
> Some additional tests have been added here in pursuit of TAMAYA-288's
> additional code coverage.
>
> You can merge this pull request into a Git repository by running:
>
> $ git pull https://github.com/peculater/incubator-tamaya TAMAYA-327
>
> Alternatively you can review and apply these changes as the patch at:
>
> https://github.com/apache/incubator-tamaya/pull/11.patch
>
> To close this pull request, make a commit to your master/trunk branch
> with (at least) the following in the commit message:
>
> This closes #11
>
> ----
> commit f63ba99ffaf4050827fc0ad88cd5df91f506956d
> Author: William Lieurance <wi...@...>
> Date: 2018-02-01T20:16:27Z
>
> TAMAYA-327: Consistent signature creating ConversionContexts
>
> Comments in ConversionContext lead me to believe that supportedFormats
> should have their order maintained after import. The HashSet that the
> Builder object uses as a backer will not accomplish that goal. This
> change converts the Builder to use ArrayLists for supportedFormats, like
> the object proper.
>
> Some additional tests have been added here in pursuit of TAMAYA-288's
> additional code coverage.
>
> ----
>
>
> ---
>
[GitHub] incubator-tamaya issue #11: TAMAYA-327: Consistent signature creating Conver...
Posted by ottlinger <gi...@git.apache.org>.
Github user ottlinger commented on the issue:
https://github.com/apache/incubator-tamaya/pull/11
@peculater as discussed on the mailing list, would you mind to finish up the pull request?
Keeping the order makes sense and doing that more consistently even more ;-)
Thanks for spotting the bug.
---
[GitHub] incubator-tamaya pull request #11: TAMAYA-327: Consistent signature creating...
Posted by johnament <gi...@git.apache.org>.
Github user johnament commented on a diff in the pull request:
https://github.com/apache/incubator-tamaya/pull/11#discussion_r165550636
--- Diff: code/api/src/main/java/org/apache/tamaya/spi/ConversionContext.java ---
@@ -145,7 +145,7 @@ public ConfigurationContext getConfigurationContext() {
/** The injection target (only set with injection used). */
private AnnotatedElement annotatedElement;
/** The ordered list of formats tried. */
- private final Set<String> supportedFormats = new HashSet<>();
+ private final List<String> supportedFormats = new ArrayList<>();
--- End diff --
would a `LinkedHashSet` work here?
---