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.

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

Modify ConfigValueLocation to use URL instead of String #180

Merged
merged 2 commits into from Mar 22, 2017
Merged

Modify ConfigValueLocation to use URL instead of String #180

merged 2 commits into from Mar 22, 2017

Conversation

jcazevedo
Copy link
Member

As suggested by @derekmorr in #178, this PR modifies ConfigValueLocation to use Path instead of String to encode the location of failures.

@derekmorr
Copy link
Collaborator

This looks good to me, but @leifwickland raised some concerns in #178 (comment)

@jcazevedo jcazevedo mentioned this pull request Mar 21, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 71.461% when pulling a04d882 on jcazevedo:config-value-location-use-path into 721603b on melrief:master.

@jcazevedo jcazevedo changed the title Modify ConfigValueLocation to use Path instead of String Modify ConfigValueLocation to use ~~Path~~/URL instead of String Mar 21, 2017
@jcazevedo jcazevedo changed the title Modify ConfigValueLocation to use ~~Path~~/URL instead of String Modify ConfigValueLocation to use URL instead of String Mar 21, 2017
@jcazevedo
Copy link
Member Author

I've modified ConfigValueLocation to use URL instead of Path, in order to be able to encode the location of configuration files inside .jar files.

@coveralls
Copy link

coveralls commented Mar 21, 2017

Coverage Status

Coverage remained the same at 71.233% when pulling 582b2b1 on jcazevedo:config-value-location-use-path into 721603b on melrief:master.

@derekmorr
Copy link
Collaborator

Path should be able to handle files in a jar file, but arguable using URL is better since ConfigFactory can load files from URLs.

@melrief
Copy link
Collaborator

melrief commented Mar 22, 2017

I prefer URL because a URL can identify any resource in the system and can be used for locations that are not necessarily files.

Copy link
Collaborator

@melrief melrief left a comment

Choose a reason for hiding this comment

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

Looks good to me

@jcazevedo
Copy link
Member Author

Since ConfigOrigin has url defined in cases where filename is null, I also think we're better off with URL.

@melrief
Copy link
Collaborator

melrief commented Mar 22, 2017

I approved this, let's wait for another approval and merge.

@derekmorr
Copy link
Collaborator

Approved.

@melrief melrief merged commit 9ee83e2 into pureconfig:master Mar 22, 2017
@jcazevedo jcazevedo deleted the config-value-location-use-path branch March 30, 2017 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants