Use named placeholder for error messages in `options.proto` #842

yevhenii-nadtochii · 2024-12-11T17:44:44Z

This PR unifies error messages format for all Spine options. Previously, the default error messages used printf-style format, but the custom ones expected placeholders instead. Now, both the custom and the default messages expect a string, which may contain placeholders in an arbitrary order.

A number of placeholders have been increased to allow specifying more detailed errors.

codecov · 2024-12-11T17:51:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.16%. Comparing base (57c83eb) to head (eaf8400).
Report is 28 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##             master     #842   +/-   ##
=========================================
  Coverage     75.16%   75.16%           
  Complexity     1178     1178           
=========================================
  Files           190      190           
  Lines          4293     4293           
  Branches        346      346           
=========================================
  Hits           3227     3227           
  Misses          935      935           
  Partials        131      131

yevhenii-nadtochii · 2024-12-12T13:37:37Z

@armiol @alexander-yevsyukov PTAL

armiol

@yevhenii-nadtochii LGTM except for some comments.

armiol · 2024-12-12T15:40:41Z

src/main/proto/spine/options.proto

@@ -668,19 +676,14 @@ message IfMissingOption {
 //         double value = 1 [(min) = {
 //             value: "0.0",
 //             exclusive: true,
-//             error_msg: "Temperature cannot reach {other}K, but provided {value}."
+//             error_msg: "The temperature cannot reach `{min.threshold}K`, but provided `{field.value}`."


I would simplify to {min.value}.

armiol · 2024-12-12T15:41:06Z

src/main/proto/spine/options.proto

-    //
-    option (default_message) = "The number must be greater than %s%s.";
+    // The default error message.
+    option (default_message) = "The field `{parent.type}.{field.name}` must be {max.comparison} {min.threshold}.";


How about {max.operator}?

armiol · 2024-12-12T15:41:39Z

src/main/proto/spine/options.proto

+    // 4. `{parent.type}` – the fully qualified name of the field declaring type.
+    // 5. `{min.threshold}` – the specified minimum threshold `value`.
+    // 6. `{min.comparison}` – if `exclusive` is set to `true`, this placeholder equals
+    //    to "greater than". Otherwise, "greater than or equal to".


As discussed today, let's use > and >=.

yevhenii-nadtochii · 2024-12-12T16:17:15Z

@armiol PTAL

Though, we still have (distinct) and (range), for which there are no custom messages, nor the default ones on the level of the option definitions. The default messages for them are still defined as string literals within validation.

armiol

@yevhenii-nadtochii LGTM.

But what if we define the corresponding behaviour for (distinct) and (range) in this PR? It would be easier to review all the proposed placeholder names together.

Please consider.

…nct)`

alexander-yevsyukov

Please see my comments. I'll review it once again after this batch is addressed.

alexander-yevsyukov · 2024-12-12T18:02:53Z

src/main/proto/spine/options.proto

 //
 message IfMissingOption {

    // The default error message.
-    option (default_message) = "A value must be set.";
+    option (default_message) = "The field `{parent.type}.{field.name}` of type `{field.type}` must have a value.";


Isn't it "the type"?

alexander-yevsyukov · 2024-12-12T18:25:55Z

src/main/proto/spine/options.proto

-    // The default error message for the field.
-    option (default_message) = "The message must have valid properties.";
+    // The default error message.
+    option (default_message) = "The field `{parent.type}.{field.path}` of type `{field.type}` is invalid. The field value: `{field.value}`.";


Why are we having here field.path instead of field.name? We refer to the immediate placement of the field in this context.

We have the combination of {parent.type}.{field.name} above.

alexander-yevsyukov · 2024-12-12T18:30:51Z

src/main/proto/spine/options.proto

-    // The default error message used when the field value is re-assigned.
-    option (default_message) = "The field `{fieldName}` already has the value `{currentValue}` and cannot be reassigned to `{proposedValue}`.";
+    // The default error message.
+    option (default_message) = "The field `{parent.type}.{field.name}` of `{field.type}` already has the value `{field.value}` and cannot be reassigned to `{field.proposed_value}`.";


Can we have "of the type {field.type}" instead of "of {field.type}"?

Here and in similar case above and below, please.

…error message

yevhenii-nadtochii · 2024-12-13T12:01:29Z

@armiol @alexander-yevsyukov PTAL

The two remaining options have got the default and custom messages support:

For the boolean (distinct) constraint, I've added a companion option (if_has_duplicates).
(range) option became a message option – RangeOption.

armiol

@yevhenii-nadtochii LGTM in general. Please see my comments.

armiol · 2024-12-13T12:08:19Z

src/main/proto/spine/options.proto

+message IfHasDuplicatesOption {
+
+    // The default error message.
+    option (default_message) = "The field `{parent.type}.{field.name}` of the type `{field.type}` must not contain duplicates. The found duplicates: `{field.duplicates}`.";


"The duplicates found:"

armiol · 2024-12-13T12:08:35Z

src/main/proto/spine/options.proto

+    //
+    //    message Blizzard {
+    //        repeated Snowflake = 1 [(distinct) = true,
+    //                                (if_has_duplicates).error_msg = "Every snowflake must be unique! The found duplicates: `{field.duplicates}`."];


Same as above.

armiol · 2024-12-13T12:08:47Z

src/main/proto/spine/options.proto

+    // 1. `{field.name}` – the field name.
+    // 2. `{field.type}` – the fully qualified name of the field type.
+    // 3. `{field.value}` – the field value (the whole collection).
+    // 4. `{field.duplicates}` – the found duplicates (elements that occur more than once).


Same as above.

src/main/proto/spine/options.proto

armiol · 2024-12-13T12:11:47Z

src/main/proto/spine/options.proto

+    //         double angle = 4 [(range).value = "(0.0..180.0)"];
+    //     }
+    //
+    // NOTE: That definition of ranges must be consistent with the type they constrain.


I would say "... they limit", or "... they describe".

armiol · 2024-12-13T12:12:17Z

src/main/proto/spine/options.proto

-//                              (if_invalid).error_msg = "The field is invalid."];
-//    }
+// It is applicable only to message fields marked with `(validate)`.
+// Repeated fields are supported.


What about maps?

alexander-yevsyukov

LGTM

yevhenii-nadtochii · 2024-12-13T13:12:40Z

@armiol PTAL

I've updated docs to (validate) and (distinct).

yevhenii-nadtochii added 6 commits December 11, 2024 16:44

Migrate IfMissionOption to placeholders

deca701

Adhere to a consistent specs format

4326b1f

Migrate MaxOption to placeholders

35c333d

Introduce {field.path} placeholder

1f2d35d

Migrate MinOption to placeholders

49d2844

Migrate PatternOption to placeholders

1e4c0ef

yevhenii-nadtochii self-assigned this Dec 12, 2024

yevhenii-nadtochii added 9 commits December 12, 2024 11:41

Migrate IfInvalidOption to placeholders

4401a87

Introduce option's scope

e8d9a29

Add more placeholders to GoesOption

4279f90

Update naming of placeholders for IfSetAgainOption

697526a

Replace tokens -> placeholders

37494d6

Unify error-message only options

ecede7d

Unify the remaining options

6805778

Bump the version -> 2.0.0-SNAPSHOT.230

3173f96

Proofread text

1c6e17a

yevhenii-nadtochii marked this pull request as ready for review December 12, 2024 13:37

yevhenii-nadtochii requested review from alexander-yevsyukov and armiol December 12, 2024 13:37

armiol requested changes Dec 12, 2024

View reviewed changes

yevhenii-nadtochii added 3 commits December 12, 2024 16:51

Move if_set_again option up, and change its number

8cf24bf

Rename threshold to value

60d55ea

Rename comparison to operator, and change its value

2030e4e

yevhenii-nadtochii requested a review from armiol December 12, 2024 16:17

armiol approved these changes Dec 12, 2024

View reviewed changes

yevhenii-nadtochii added 2 commits December 12, 2024 18:10

Add IfHasDuplicates option to specify the error message for `(disti…

5a37fba

…nct)`

Introduce RangeOption

b709e17

yevhenii-nadtochii added 2 commits December 12, 2024 18:23

Add IfHasDuplicatesOption to field options

3abe8be

Update docs

b4d5373

alexander-yevsyukov requested changes Dec 12, 2024

View reviewed changes

yevhenii-nadtochii added 2 commits December 13, 2024 11:32

Fix the article

6b5105e

Use the direct field.name instead of field.path for (validate) …

5cfa325

…error message

yevhenii-nadtochii requested review from alexander-yevsyukov and armiol December 13, 2024 12:01

armiol requested changes Dec 13, 2024

View reviewed changes

alexander-yevsyukov approved these changes Dec 13, 2024

View reviewed changes

yevhenii-nadtochii added 3 commits December 13, 2024 13:24

Proofread text

9f70e7b

Update specs to (validate) specs

4ecba08

Add more details to (distinct) spec

eaf8400

armiol approved these changes Dec 13, 2024

View reviewed changes

yevhenii-nadtochii merged commit 5388f69 into master Dec 13, 2024
7 checks passed

yevhenii-nadtochii deleted the error-message-placeholders branch December 13, 2024 13:29

yevhenii-nadtochii mentioned this pull request Dec 13, 2024

Properly migrate the field type in options.proto #843

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use named placeholder for error messages in `options.proto` #842

Use named placeholder for error messages in `options.proto` #842

yevhenii-nadtochii commented Dec 11, 2024 •

edited

Loading

codecov bot commented Dec 11, 2024 •

edited

Loading

yevhenii-nadtochii commented Dec 12, 2024 •

edited

Loading

armiol left a comment

armiol Dec 12, 2024

armiol Dec 12, 2024

armiol Dec 12, 2024

yevhenii-nadtochii commented Dec 12, 2024

armiol left a comment

alexander-yevsyukov left a comment

alexander-yevsyukov Dec 12, 2024

alexander-yevsyukov Dec 12, 2024

alexander-yevsyukov Dec 12, 2024

alexander-yevsyukov Dec 12, 2024

alexander-yevsyukov Dec 12, 2024

yevhenii-nadtochii commented Dec 13, 2024

armiol left a comment

armiol Dec 13, 2024

armiol Dec 13, 2024

armiol Dec 13, 2024

armiol Dec 13, 2024

armiol Dec 13, 2024

alexander-yevsyukov left a comment

yevhenii-nadtochii commented Dec 13, 2024

Use named placeholder for error messages in options.proto #842

Use named placeholder for error messages in options.proto #842

Conversation

yevhenii-nadtochii commented Dec 11, 2024 • edited Loading

codecov bot commented Dec 11, 2024 • edited Loading

Codecov Report

yevhenii-nadtochii commented Dec 12, 2024 • edited Loading

armiol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yevhenii-nadtochii commented Dec 12, 2024

armiol left a comment

Choose a reason for hiding this comment

alexander-yevsyukov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yevhenii-nadtochii commented Dec 13, 2024

armiol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-yevsyukov left a comment

Choose a reason for hiding this comment

yevhenii-nadtochii commented Dec 13, 2024

Use named placeholder for error messages in `options.proto` #842

Use named placeholder for error messages in `options.proto` #842

yevhenii-nadtochii commented Dec 11, 2024 •

edited

Loading

codecov bot commented Dec 11, 2024 •

edited

Loading

yevhenii-nadtochii commented Dec 12, 2024 •

edited

Loading