Use placeholder-based messages in ConstraintViolation
#173
Conversation
…o return the template
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #173 +/- ##
============================================
+ Coverage 33.08% 33.55% +0.47%
+ Complexity 350 349 -1
============================================
Files 139 142 +3
Lines 2805 2834 +29
Branches 228 234 +6
============================================
+ Hits 928 951 +23
- Misses 1808 1814 +6
Partials 69 69 |
| @@ -46,6 +47,7 @@ internal class ErrorMessageSpec { | |||
| val error = exception.asMessage() | |||
| error.constraintViolationList.size shouldBe 1 | |||
| val violation = error.getConstraintViolation(0) | |||
| violation.msgFormat shouldBe "Expected less than 100 Cents per one Dollars, but got 101." | |||
| val message = violation.message.format() | |||
| message shouldBe "Expected less than 100 Cents per one Dollars, but got 101." | |||
| paramList shouldBe expectedParams | ||
| message.withPlaceholders shouldBe template(field.index + 1) | ||
| message.placeholderValueMap shouldContainExactly mapOf( | ||
| "field.name" to fieldName, |
There was a problem hiding this comment.
As discussed vocally, let's have something less prone to typos for operating the placeholder names than just string literals. E.g. constants, and probably some API on top of them, such as extension functions for MessageTemplate.Builder (for Kotlin API, at least).
| * Creates a new [TemplateString] with the given [value], which doesn't | ||
| * contain placeholders. | ||
| */ | ||
| internal fun withoutPlaceholders(value: String): TemplateString = templateString { |
There was a problem hiding this comment.
I would shorten to plainString.
There was a problem hiding this comment.
doesn't
Let's use full form in the documentation: "does not". This is a general recommendation for technical docs because this way it's more visible.
| val formatted = ViolationText.of(violation).toString() | ||
|
|
||
There was a problem hiding this comment.
Please remove these leading space characters.
| "field.type" to fieldType, | ||
| "field.value" to "$value1", | ||
| "field.proposed_value" to "$value2", | ||
| "parent.type" to parentType |
There was a problem hiding this comment.
Please introduce constants for the placeholder names. We'll need them for finding usages.
|
@armiol @alexander-yevsyukov PTAL As for now, we don't need |
There was a problem hiding this comment.
@yevhenii-nadtochii LGTM in general. See my comments though.
Let's try one more thing. How about converting MessageValidator.java into Kotlin, and creating the convenient extensions, as per my past proposal?
For instance, an extension function
TemplateString.Builder.withField(field: FieldDeclaration)would allow us to write the following code:
var field = constraint.field();
// ...
violation.message
.toBuilder()
.withField(field)
// ^^^ Sets all three values:
// FIELD_NAME("field.name"),
// FIELD_VALUE("field.value"),
// FIELD_TYPE("field.type"),
It is both shorter, and less prone to errors, as it accepts FieldDeclarations, not Strings.
| * placeholder_value = { "dog.name": "Fido" } | ||
| * ``` | ||
| * | ||
| * This method will return "My dog's name is Fido". |
| */ | ||
| public fun TemplateString.format(): String { | ||
| checkPlaceholdersHasValue(withPlaceholders, placeholderValueMap) { | ||
| "Can not format the given `TemplateString`: `$withPlaceholders`. " + |
|
@armiol PTAL |
This PR deprecates
printferror message string inConstraintViolationin favor ofTemplateString. The newly created type expects a string template with${...}placeholder, and a map of placeholder values. This matches more to what we define for options within Proto definitions.Please note, the full support of recently introduced format of message placeholders is not fully implemented in this PR. This is the first step towards it.
(set_once)was migrated completely because it stands separately from theruleframework. Runtime validation was updated in a compatible way, though, as for now, we have no intention to fully support the introduced message templates. The codegen-based validation is to be migrated in the upcoming PRs.