-
-
Notifications
You must be signed in to change notification settings - Fork 419
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
[config] Add config validator for parameter options #2691
[config] Add config validator for parameter options #2691
Conversation
} | ||
|
||
// Option values are a string, so we can do a simple compare | ||
if (!param.getOptions().stream().map(o -> o.getValue()).anyMatch(v -> v.equals(value.toString()))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use noneMatch
// Option values are a string, so we can do a simple compare | ||
if (!param.getOptions().stream().map(o -> o.getValue()).anyMatch(v -> v.equals(value.toString()))) { | ||
MessageKey messageKey = MessageKey.OPTIONS_VIOLATED; | ||
return new ConfigValidationMessage(param.getName(), messageKey.defaultMessage, messageKey.key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to add the list of allowed options here. This might be helpful to correct the setting.
Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
1501b47
to
2e53959
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A general comment: IMO ConfigDescriptionParameterValidatorFactory
should be removed. There is no need for it, it just returns a new instance which could also be achieved by inserting new ...
in the map. But maybe we should put that in another PR (like removing the unnecessary checks we talked about in #2690.
// =========================================================================== | ||
|
||
@Test | ||
public void assertValidationThrowsNoExceptionForAllowedLimitedParameterOption() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For other validations we assert valid parameters pass the validation by adding a valid value in setUp
. To keep it consistent, we should do the same here and only keep a separate test for the invalid one.
Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the validator!
Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de> GitOrigin-RevId: 9cda2c8
Depends on #2690
Signed-off-by: Christoph Weitkamp github@christophweitkamp.de