Encourage to avoid using `(required)` field option for numeric and boolean types #266

serhii-lekariev · 2018-12-21T13:23:26Z

This PR closes #197.

Before, attempting to use (required) = true on all numeric types and the boolean type resulted in false-positive violations of ValidatingException - the numeric fields were compared against 0 and boolean fields were compared against false, and if they were equal to those default values, the fields were considered unset.

This is semantically incorrect, because both 0 and false, while being a default, are valid values nonetheless.

Now, when trying to validate required fields of the following numeric types:

int32
int64
float
double

or a required field of type bool, a warning is produced, telling that doing so is incorrect.

doing so requires comparing it against a default value, which in case of numeric types is `0` - a perfectly valid field value

cannot be such because of its type

serhii-lekariev · 2018-12-21T13:24:44Z

@dmdashenkov, PTAL.

dmdashenkov

@serhii-lekariev, please see my comments.

dmdashenkov · 2018-12-24T09:45:45Z

base/src/main/java/io/spine/validate/FieldValidator.java

+     *
+     * @return whether a field that is being validated can be {@code required}
+     */
+    protected abstract boolean requiredAllowed();


This method name is complicated. It uses 2 past form verbs. Please think of a better name.

dmdashenkov · 2018-12-24T09:45:58Z

base/src/main/java/io/spine/validate/FieldValidator.java

+    /**
+     * Checks whether a field can be {@code (required) = true}.
+     *
+     * Validators can define that for their respective types


<p> is missing.

dmdashenkov · 2018-12-24T09:46:52Z

base/src/main/java/io/spine/validate/FieldValidator.java

        return result;
    }

+    private void canBeRequired() {


The name does not reflect the action. I'd expect a method starting with can to return a boolean. Please rename.

dmdashenkov · 2018-12-24T09:47:07Z

base/src/main/java/io/spine/validate/FieldValidator.java

     */
    @SuppressWarnings("ConstantConditions")
-        // `isFirstTimeSet` controls for `null`s.
+    // `isFirstTimeSet` controls for `null`s.


Please undo this change.

dmdashenkov · 2018-12-24T09:47:39Z

base/src/main/java/io/spine/validate/FieldValidator.java

     * <p>If the field is repeated, it must have at least one value set, and all its values
     * must be valid.
     *
+     * <p>If the field is of a numeric type, it's always considered set.


What about bools?

dmdashenkov · 2018-12-24T09:49:43Z

base/src/main/java/io/spine/validate/StringFieldValidator.java

+
+    @Override
+    protected boolean requiredAllowed() {
+        return true;


All the overloads of this method return a constant. Please consider passing the value in a super(...) ctor call.

dmdashenkov · 2018-12-24T09:59:12Z

base/src/main/java/io/spine/validate/NumberFieldValidator.java

+     * @implNote Attempting to define whether a numeric field is set or not is semantically
+     * incorrect, because the way to define whether a value is set or not is to compare it against
+     * its default value, which for numeric fields is {@code 0}, which is sometimes a desired and
+     * valid value for a field.


This one sentence is too long. Please split it up and simplify. Also, I'm not sure if <p> paragraphs are supported in @implNote.

# Conflicts: # base/src/main/java/io/spine/validate/FieldValidator.java

from abstract methods to a field initialized in the constructor

codecov · 2018-12-25T14:03:45Z

Codecov Report

Merging #266 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #266      +/-   ##
============================================
+ Coverage     74.74%   74.76%   +0.02%     
  Complexity      689      689              
============================================
  Files           302      302              
  Lines          8236     8240       +4     
  Branches        551      551              
============================================
+ Hits           6156     6161       +5     
  Misses         1944     1944              
+ Partials        136      135       -1

with tests

`validateOwnRules` method, since it's already done in the `FieldValidator#validate()`

serhii-lekariev · 2018-12-25T15:23:15Z

@dmdashenkov, PTAL.

dmdashenkov

@serhii-lekariev, please see some new comments.

dmdashenkov · 2018-12-26T10:29:40Z

base/src/main/java/io/spine/validate/FieldValidator.java

+    private void checkCanBeRequired() {
+        boolean fieldIsRequired = isRequiredField();
+        if (!canBeRequired && fieldIsRequired) {
+            String messageFormat = "Fields of type %s can't be declared as `(required)`.";


Suggested change

String messageFormat = "Fields of type %s can't be declared as `(required)`.";

String messageFormat = "Fields of type %s shouldn't be declared as `(required)`.";

dmdashenkov · 2018-12-26T10:32:05Z

base/src/test/java/io/spine/validate/FloatFieldValidatorTest.java

    FloatFieldValidatorTest() {
-        super(HALF, -HALF, new FloatFieldValidator(FieldValue.of(HALF, fieldContext)));
+        super(HALF, -HALF, new FloatFieldValidator(FieldValue.of(HALF, fieldContext)),
+              requiredFieldValidator());


Please allocate each next param on a new line.

dmdashenkov · 2018-12-26T10:34:46Z

base/src/main/java/io/spine/validate/FieldValidator.java

+                                  .descriptor()
+                                  .getType()
+                                  .name();
+            log().warn(messageFormat, typeName);


Please use Logging._warn() and similar methods instead.

dmdashenkov · 2018-12-26T10:35:45Z

base/src/main/java/io/spine/validate/FieldValidator.java

+            String typeName = this.field()
+                                  .descriptor()
+                                  .getType()
+                                  .name();


This is ugly. Please use FieldDeclaration methods. If there is no such method, please add one.

serhii-lekariev · 2018-12-26T18:07:08Z

@dmdashenkov, PTAL again.

dmdashenkov

@serhii-lekariev, LGTM after a single comment is addressed.

dmdashenkov · 2018-12-27T10:42:43Z

base/src/main/java/io/spine/validate/BooleanFieldValidator.java

-        if (isRequiredField()) {
-            log().warn("'required' option not allowed for boolean field");
-        }
+        // NOP


Please document the reason.

serhii-lekariev added 3 commits December 20, 2018 18:24

Remove a way to determine whether a numeric is set or not because

d5437b7

doing so requires comparing it against a default value, which in case of numeric types is `0` - a perfectly valid field value

Add a warning that fires upon finding a field that is required but

d2979dd

cannot be such because of its type

Clarify the warning produced during validation of required fields

d81ba0a

serhii-lekariev self-assigned this Dec 21, 2018

serhii-lekariev requested a review from dmdashenkov December 21, 2018 13:23

dmdashenkov suggested changes Dec 24, 2018

View reviewed changes

serhii.lekariev and others added 2 commits December 24, 2018 18:17

Merge branch 'set_once_message_option' into required_option_for_numbers

de96fb9

# Conflicts: # base/src/main/java/io/spine/validate/FieldValidator.java

Log a warning if a field that cannot be required is declared as such

ab1797c

serhii-lekariev changed the base branch from set_once_message_option to master December 25, 2018 13:35

serhii-lekariev and others added 2 commits December 25, 2018 15:37

Merge branch 'master' into required_option_for_numbers

f1b3d5b

Move the definition of whether a field can be required

977fcea

from abstract methods to a field initialized in the constructor

serhii-lekariev added 2 commits December 25, 2018 17:03

Cover warnings upon finding a required numeric or boolean field

0723d0c

with tests

Remove warning logging from the BooleanFieldValidators

48bc4e3

`validateOwnRules` method, since it's already done in the `FieldValidator#validate()`

dmdashenkov suggested changes Dec 26, 2018

View reviewed changes

Improve wording and formatting

48287a5

SpineEventEngine deleted a comment Dec 26, 2018

dmdashenkov approved these changes Dec 27, 2018

View reviewed changes

Merge branch 'master' into required_option_for_numbers

a014b5c

SpineEventEngine deleted a comment Dec 30, 2018

Document a method

7363fae

SpineEventEngine deleted a comment Jan 2, 2019

serhii-lekariev merged commit 8ee75f1 into master Jan 2, 2019

serhii-lekariev deleted the required_option_for_numbers branch January 2, 2019 10:08

	String messageFormat = "Fields of type %s can't be declared as `(required)`.";
	String messageFormat = "Fields of type %s shouldn't be declared as `(required)`.";

Encourage to avoid using (required) field option for numeric and boolean types #266

Encourage to avoid using (required) field option for numeric and boolean types #266

Uh oh!

Conversation

serhii-lekariev commented Dec 21, 2018

Uh oh!

serhii-lekariev commented Dec 21, 2018

Uh oh!

dmdashenkov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Dec 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

serhii-lekariev commented Dec 25, 2018

Uh oh!

dmdashenkov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhii-lekariev commented Dec 26, 2018

Uh oh!

dmdashenkov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Encourage to avoid using `(required)` field option for numeric and boolean types #266

Encourage to avoid using `(required)` field option for numeric and boolean types #266

codecov bot commented Dec 25, 2018 •

edited

Loading