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

ConfigValueConfigSource to retrieve Configuration value location. #262

Merged
merged 1 commit into from
Mar 31, 2020

Conversation

radcortez
Copy link
Member

@radcortez radcortez commented Mar 13, 2020

This is just a prototype attempt to provide a way to retrieve the Configuration Value location and origin.

@radcortez radcortez requested a review from dmlloyd March 13, 2020 16:39
@radcortez radcortez linked an issue Mar 13, 2020 that may be closed by this pull request
Copy link
Contributor

@dmlloyd dmlloyd left a comment

Choose a reason for hiding this comment

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

I think we might need to explore some different ideas for this.

@radcortez
Copy link
Member Author

Sure. This was not meant to be a final implementation, but just to get the discussion going. For instance, the Properties API is somehow limited, so if we want to load the keys and values, plus keep track of the line number, I believe we need to write our own Properties Loader.

Once that one is in place, then we can actually have a single lookup per key and improve the rest of the code.

Do you think we should do it?

@dmlloyd
Copy link
Contributor

dmlloyd commented Mar 13, 2020

Sure. This was not meant to be a final implementation, but just to get the discussion going. For instance, the Properties API is somehow limited, so if we want to load the keys and values, plus keep track of the line number, I believe we need to write our own Properties Loader.

Once that one is in place, then we can actually have a single lookup per key and improve the rest of the code.

Do you think we should do it?

Yes, I think we should.

@radcortez
Copy link
Member Author

Ok, I'll work on a custom implementation for Properties that allows us to do this more cleanly.

@radcortez radcortez changed the title TracedConfigSource to retrieve Configuration value location. ConfigValueConfigSource to retrieve Configuration value location. Mar 16, 2020
@radcortez
Copy link
Member Author

Ok, I've redone the PR to take into account our discussions.

dmlloyd
dmlloyd previously approved these changes Mar 24, 2020
Copy link
Contributor

@dmlloyd dmlloyd left a comment

Choose a reason for hiding this comment

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

I'm OK with this now, but note that we're missing a lot of JavaDoc here.

@radcortez
Copy link
Member Author

I'm writing the documentation as well :)

@radcortez
Copy link
Member Author

@dmlloyd actually just pushed it now. I can also add the javadoc portions.

@radcortez
Copy link
Member Author

@dmlloyd Check if this is better now :)

@radcortez
Copy link
Member Author

Ok, so I hope I've fixed all of your comments. Thank you :)

@radcortez
Copy link
Member Author

@dmlloyd Do you think this is now ready to go in? :)

@radcortez
Copy link
Member Author

@dmlloyd I've removed the logging interceptor now, which was the main cause of the discussion.

Everything else is how we discussed and implemented. Rebased the PR. Hope it is good to go now.

@radcortez
Copy link
Member Author

Thanks @dmlloyd :)

@radcortez radcortez merged commit 31a3452 into smallrye:master Mar 31, 2020
@radcortez radcortez deleted the #248 branch April 2, 2020 11:38
@radcortez radcortez added this to the 1.7.1 milestone Apr 16, 2020
@radcortez radcortez linked an issue Apr 16, 2020 that may be closed by this pull request
* ConfigValueConfigSource tries to make this possible.
* <p>
*
* Ideally, this should move the the MicroProfile Config API, once the concept is well-proven.

Choose a reason for hiding this comment

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

typo: move the the MicroProfile
..move to the..

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.

Configuration value location
3 participants