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

Config direct values #134

Merged
merged 2 commits into from
Nov 1, 2018
Merged

Config direct values #134

merged 2 commits into from
Nov 1, 2018

Conversation

tomas-langer
Copy link
Member

No description provided.

@tomas-langer
Config fixes to value accessed on tree node.
bf18afc
@tomas-langer
Next steps in supporting direct values also for list nodes and object…
0a3046f 
… nodes.

Returning empty optional for asOptional for any node type
Throwing MissingValueException where the value is missing rather than ConfigMappingException
Refactored tests
 - changed tests to conform to the above
 - extracted duplicate code to a common ancestor
 - fixed issues (methods that were testing supplier were not correctly implemented)
@tomas-langer tomas-langer self-assigned this Oct 29, 2018
@tomas-langer tomas-langer requested a review from tjquinno October 29, 2018 20:21
@tjquinno
Copy link
Member

I'll be approving this in a moment.

A couple general comments:

  1. I know you mentioned in our recent meeting that you felt config contains too many unit tests, and in this PR you've removed quite a few. I don't see that the removed tests are affected by the feature enhancement these changes implement, so ideally that wouldn't that clean-up be in a separate PR with test clean-up as its purpose (and name) to be clearer?

  2. Many (all?) of the removed tests exercise the Config.asXXX methods. Most of those methods are very simple and so unit tests for each might be less important than other tests. But given that those tests were already there and because they test methods exposed in the central Config interface, why remove them now?

tjquinno
tjquinno approved these changes Oct 31, 2018
View reviewed changes
Copy link
Member

@tjquinno tjquinno left a comment

Choose a reason for hiding this comment

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

Good to go.

@tomas-langer
Copy link
Member Author

The unit tests were not removed, only moved to an abstract class, as they were common for both Object and List configuration implementation.
The number of tests that are executed during a build is unchanged.

@tomas-langer tomas-langer merged commit 18dd495 into helidon-io:master Nov 1, 2018
@tomas-langer tomas-langer deleted the config-direct-values branch November 1, 2018 12:43
@tjquinno
Copy link
Member

tjquinno commented Nov 1, 2018

Sorry I didn't pick up on the move of the test code. Thanks.

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

@tjquinno tjquinno tjquinno approved these changes

Assignees

@tomas-langer tomas-langer

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tomas-langer @tjquinno