Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

wants to merge 1 commit into from

Conversation

rice668
Copy link

@rice668 rice668 commented Nov 6, 2017 •
edited
Loading

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
Copy link

Coverage Status

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

@rice668 rice668 force-pushed the COLLECTIONS-664 branch 3 times, most recently from 1b6ca96 to 9434fd8 Compare November 6, 2017 09:36
@rice668
Copy link
Author

rice668 commented Nov 6, 2017

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

@rice668
Copy link
Author

rice668 commented Nov 6, 2017

The CI error seems does not relevant to this PR.

@kinow
Copy link
Member

kinow commented Nov 6, 2017

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
Copy link
Author

rice668 commented Nov 6, 2017

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
Copy link
Member

kinow commented Nov 6, 2017

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

@rice668
Copy link
Author

rice668 commented Nov 6, 2017

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

garydgregory
garydgregory reviewed Jan 3, 2018
View reviewed changes
Copy link
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

kinow commented May 5, 2019

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
garydgregory requested changes May 13, 2019
View reviewed changes
Copy link
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garydgregory garydgregory garydgregory requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rice668 @coveralls @kinow @Claudenw @garydgregory