[COLLECTIONS-664] Add a class that extend a load method which accept …

[rice668]

For example, In some interface, needs we have a java.util.Properties as a parameter. In common case, we have different profiles in maven under different environments that we can random selection. And use the current classloader, usually, belongs to AppclassLoader to get its Inputstream. I propose we should add a functionality that can directly read a file name and return a java.util.Properties as a return value.

[coveralls]

Coverage increased (+0.006%) to 86.622% when pulling 3ee56fd on zhangminglei:COLLECTIONS-664 into 0b1460d on apache:master.

[rice668]

Hello, @kinow, Could you please take a look on this PR? Thanks!

[rice668]

The CI error seems does not relevant to this PR.

[kinow]

Hi @zhangminglei !

Sure. Few questions from reviewing the JIRA ticket and the pull request.

• Why Commons Collections? The SortedProperties uses Java collection classes to sort its keys. But FileProperties doesn't seem to have much to do with Java collections.
• The name of the class doesn't really suggest what it does. Unless there are other use cases, we could be more specific about the class name.
• However, I suspect this code belongs more to the user code, and not really to the low level library. Might be better to update the ticket to Commons Configuration maybe (?), and send a message to the development mailing list in case you don't hear back within some days.

The code is well written, and with tests :-) we just need to review the use case, and which component could host it.

Hope it helps
Bruno

[rice668]

Hello, @kinow Thanks for your reply!

• Why Commons Collections?. I am not sure which is the best address I should put it. At the beginning, I created the JIRA under Commons Configuration, like you said at the second point. Then I changed my mind.

• As refers to the file name, yea. I think I should give an appropriate name for it, instead of FileProperties.

• I agree with you at this point, before I did this, I searched for a long time for this functionality in Commons Configuration. But I could not find it.

Yes. I will send a message to dev mail list.

Thanks
Minglei

[kinow]

Thanks for the clear response Minglei! And thanks for your contribution. Let's see what others think 👍

[rice668]

Thanks @kinow . I would move this PR to Commons/Configuration. I just not sure which component is the best to put it.

[garydgregory]

If this is only about reading, I could maybe see a new class called PropertiesFactory that can read not just from a java.io.File but also other inputs like a java.nio.file.Path. Thoughts?

[kinow]

Just had a play with Commons Configuration, and they have some code for handling loading Properties (I think I used version 1, version 2 has builders...).

I suspect this feature would at their PropertiesConfiguration, and if it doesn't do the same as what is suggested here, then implement a new PropertiesReader I think (near the bottom of PropertiesConfiguration, there are some readers in there). Or maybe through some other part of Commons Configuration API.

Haven't looked at the issues in Commons Configuration, but maybe @garydgregory 's suggestion of supporting Path's too could be a good enhancement to this proposal.

[garydgregory]

This is too confusing as is IMO.

This is a factory class. It does not need to extend Properties.

Its factory method can be static. The load method is too mysterious since it does not mention resources.

The method also lacks Javadocs.

The stream is opened but not closed, a try-with-resources block is best in this case.

I can take a better look later.

[Claudenw]

@zhangminglei did you find a home for this contribution. I suspect that it does belong in commons-configuration but want to know if you got it added.