[configuration2] Mismatch between getList doc and implementation

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

[configuration2] Mismatch between getList doc and implementation

Roman Zaynetdinov
Hi,

I am switching to commons-configuration2 and trying to use typed getList
method. I have noticed that javadoc doesn't correspond to the actual
implementation.

ImmutableConfiguration.java:

/**
 * Gets a list of typed objects associated with the given configuration key
 * returning an empty list if the key doesn't map to an existing object.
 * ...
 */
<T> List<T> getList(Class<T> cls, String key);

But implementation uses null as a default value. AbstractConfiguration.java:

public <T> List<T> getList(final Class<T> cls, final String key)
{
    return getList(cls, key, null);
}

Should it use new ArrayList<>() instead of null? Similar to untyped getList
method?

- Roman
Reply | Threaded
Open this post in threaded view
|

Re: [configuration2] Mismatch between getList doc and implementation

garydgregory
Hi Roman,

You can try the change and see if it breaks any existing tests. Feel free
to create a PR even if it just to make the Javadoc match the code. I've not
studied this part of the code in a while, so maybe someone else can opine.

Gary


On Fri, Feb 12, 2021, 18:30 Roman Zaynetdinov <[hidden email]>
wrote:

> Hi,
>
> I am switching to commons-configuration2 and trying to use typed getList
> method. I have noticed that javadoc doesn't correspond to the actual
> implementation.
>
> ImmutableConfiguration.java:
>
> /**
>  * Gets a list of typed objects associated with the given configuration key
>  * returning an empty list if the key doesn't map to an existing object.
>  * ...
>  */
> <T> List<T> getList(Class<T> cls, String key);
>
> But implementation uses null as a default value.
> AbstractConfiguration.java:
>
> public <T> List<T> getList(final Class<T> cls, final String key)
> {
>     return getList(cls, key, null);
> }
>
> Should it use new ArrayList<>() instead of null? Similar to untyped getList
> method?
>
> - Roman
>
Reply | Threaded
Open this post in threaded view
|

Re: [configuration2] Mismatch between getList doc and implementation

Roman Zaynetdinov
In reply to this post by Roman Zaynetdinov
Hi Gary,

I somehow didn't get your response in my email so responding to my own
message instead.

I have found the tests for this case here
https://github.com/apache/commons-configuration/blob/master/src/test/java/org/apache/commons/configuration2/TestAbstractConfigurationBasicFeatures.java#L898-L907
Sadly for me it means that the method should be returning null. I was
hoping otherwise...

Changed doc in https://github.com/apache/commons-configuration/pull/100

- Roman

> You can try the change and see if it breaks any existing tests. Feel free
> to create a PR even if it just to make the Javadoc match the code. I've
not
> studied this part of the code in a while, so maybe someone else can opine.
>
> Gary

On Fri, 12 Feb 2021 at 15:58, Roman Zaynetdinov <
[hidden email]> wrote:

> Hi,
>
> I am switching to commons-configuration2 and trying to use typed getList
> method. I have noticed that javadoc doesn't correspond to the actual
> implementation.
>
> ImmutableConfiguration.java:
>
> /**
>  * Gets a list of typed objects associated with the given configuration key
>  * returning an empty list if the key doesn't map to an existing object.
>  * ...
>  */
> <T> List<T> getList(Class<T> cls, String key);
>
> But implementation uses null as a default value.
> AbstractConfiguration.java:
>
> public <T> List<T> getList(final Class<T> cls, final String key)
> {
>     return getList(cls, key, null);
> }
>
> Should it use new ArrayList<>() instead of null? Similar to untyped
> getList method?
>
> - Roman
>