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.

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

Implement the API modifications #129

Conversation

ArturAssisComp
Copy link
Contributor

Connection with issue(s)

Close #128

Connected to #119 #116

Solution description

Screenshots or Videos

To Do

  • Read contributing guide
  • Check the original issue to confirm it is fully satisfied
  • Add solution description to help guide reviewers
  • Add unit test to verify new or fixed behaviour
  • If apply, add documentation to code properties and package readme

Copy link

codecov bot commented Aug 13, 2024

Codecov Report

Attention: Patch coverage is 97.79559% with 11 lines in your changes missing coverage. Please review.

Project coverage is 98.74%. Comparing base (e8da6fc) to head (1834d8e).
Report is 96 commits behind head on refactor_2024_11.

Files with missing lines Patch % Lines
lib/src/validators/string_validators.dart 90.14% 7 Missing ⚠️
lib/src/validators/collection_validators.dart 96.87% 2 Missing ⚠️
lib/src/validators/datetime_validators.dart 95.83% 1 Missing ⚠️
lib/src/validators/path_validators.dart 96.00% 1 Missing ⚠️
Additional details and impacted files
@@                  Coverage Diff                  @@
##           refactor_2024_11     #129       +/-   ##
=====================================================
+ Coverage             86.44%   98.74%   +12.29%     
=====================================================
  Files                   109      111        +2     
  Lines                  1483     1510       +27     
=====================================================
+ Hits                   1282     1491      +209     
+ Misses                  201       19      -182     
Flag Coverage Δ
unittests 98.74% <97.79%> (+12.29%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@deandreamatias
Copy link
Contributor

@ArturAssisComp before implement all validators, I think that is better create a documentation on readme to be discuss about.
I don't sure about the "duplicate" all validators to add a Elementary behaviour

@ArturAssisComp
Copy link
Contributor Author

@deandreamatias ok, I will do that. I implemented only the bool validators. I am using the version already implemented to make use of the tests, but today I will implement a prototype of the FormBuilderValidator using the new API to discuss the idea. Thanks for the feedback.

@ArturAssisComp
Copy link
Contributor Author

ArturAssisComp commented Aug 21, 2024

I added a section at the end of the readme for the discussion. But it can also be discussed here. I made a video documentation about what I did too.

https://youtu.be/zgzHehmsFd0?si=AE-zE4ESETPFP9xU

@deandreamatias
Copy link
Contributor

I taken a look on this propouse. My thougths:

    1. Is really hard to catch all cases and logic on the API propouse. Make sense the abstract concept, but I think that the implementation can be more simple for developer experience.
      Maybe can use FormBuilderValidators.compose to add required and optional validators. Like FormBuilderValidators.compose(required: [validators], optional: [another validators])
  1. The name ValidatorBuilder is really necessary? On Flutter, Builder word usually is used to widgets that have a builder property.
  2. The types validators like IsNumericElementaryValidator, I think like a nice thing to have, is an improvement to reuse code.
  3. Thinks like
 ValidatorBuilder.numeric(
          errorText: 'La edad debe ser numérica.',
          and: <BaseElementaryValidator<num, dynamic>>[
            ValidatorBuilder.max(70),
          ])

could be simplify. A option is use ValidatorBuilder.numeric().and(ValidatorBuilder.max(70)) or even improve andc reate something like ValidatorBuilder.numeric() && ValidatorBuilder.max(70)

@ArturAssisComp
Copy link
Contributor Author

ArturAssisComp commented Aug 23, 2024

    1. Is really hard to catch all cases and logic on the API propouse. Make sense the abstract concept, but I think that the implementation can be more simple for developer experience.
      Maybe can use FormBuilderValidators.compose to add required and optional validators. Like FormBuilderValidators.compose(required: [validators], optional: [another validators])

I will investigate that with a prototype.

  1. The name ValidatorBuilder is really necessary? On Flutter, Builder word usually is used to widgets that have a builder property.

That is right. I forgot about the builder semantics that is specific to Flutter. I will think about another name.

  1. Thinks like
 ValidatorBuilder.numeric(
          errorText: 'La edad debe ser numérica.',
          and: <BaseElementaryValidator<num, dynamic>>[
            ValidatorBuilder.max(70),
          ])

could be simplify. A option is use ValidatorBuilder.numeric().and(ValidatorBuilder.max(70)) or even improve andc reate something like ValidatorBuilder.numeric() && ValidatorBuilder.max(70)

I think that would be a good idea. I will investigate with prototypes too.

@ArturAssisComp
Copy link
Contributor Author

Tomorrow I will give a better comment on the new API prototype. But it is already implemented for the examples. Fell free to take a look at the new_api_prototype.dart if you want. Otherwise, wait for my comments.
I also did a draft benchmark on a password checker comparing the new api with the old one.

image

@ArturAssisComp
Copy link
Contributor Author

ArturAssisComp commented Aug 31, 2024

Conventions: I will call the API that is being used as "original api", the previous proposal as "initial prototype" and this prototype as "new prototype".
About the new prototype:

  • The idea was to simply decouple the required, optional and type validators from the other validators. Those validators are considered intermediate validators, thus they receive as an argument the next validator. There are 'and' and 'or' compositions that make it easier to create complex logical combination of multiple validators.
  • It maintains the functionality of the original and the initial prototype
// All the use cases of the example/lib/main.dart were reproduced using the new prototype. They are in example/lib/app_refactoring_main.dart
// Some examples are
// Original api
FormBuilderValidators.compose(<FormFieldValidator<String>>[
    /// Makes this field required
    FormBuilderValidators.required(),

     /// Ensures the value entered is numeric - with custom error message
     FormBuilderValidators.numeric(
         errorText: 'La edad debe ser numérica.',
     ),

     /// Sets a maximum value of 70
     FormBuilderValidators.max(70),

     /// Include your own custom `FormFieldValidator` function, if you want
     /// Ensures positive values only. We could also have used `FormBuilderValidators.min( 0)` instead
     (String? val) {
          if (val != null) {
              final int? number = int.tryParse(val);
              if (number == null) return null;
              if (number < 0) return 'We cannot have a negative age';
          }
          return null;
     }
]);

// New prototype
/// Makes this field required (req is an alias for isRequired)
req(
    /// Ensures the value entered is numeric - with custom error message
    isNum(
         and([
             /// Sets a maximum value of 70
             max(70),
             /// Include your own custom `FormFieldValidator` function, if you want
             (num value) {
                 if (value < 0) return 'We cannot have a negative age';
                 return null;
             },
         ]),
         isNumMessage: 'La edad debe ser numérica.',
    ),
);
  • It removes redundancy of computations for composition of validators. Most of them recalculate if it is null/empty and check the type. As a direct result, it improves the response time of composed validators.
  • It maintains the simplicity of the original api.
  • It is completely compatible with the original api
/// The previous example but using the compose method from the original api
FormBuilderValidators.compose(<FormFieldValidator<String>>[
    req(isNum(max(70), isNumMessage: 'La edad debe ser numérica.')),
    /// Include your own custom `FormFieldValidator` function, if you want
    /// Ensures positive values only. We could also have used `FormBuilderValidators.min( 0)` instead
    (String? val) {
        if (val != null) {
            final int? number = int.tryParse(val);
            if (number == null) return null;
            if (number < 0) return 'We cannot have a negative age';
        }
        return null;
    }
])
  • It maintains the easiness for creating custom validators like it was for the original api
  • It makes it easier for composing validators using and and or logical operators.
// Composition with and
// Original api
FormBuilderValidators.compose(<FormFieldValidator<String>>[
    FormBuilderValidators.required(),
    FormBuilderValidators.numeric(),
    FormBuilderValidators.min(0),
    FormBuilderValidators.max(120),
]),
// New prototype
isRequired(isNum(and([min(0), max(120)])));

// Composition with or and and: The error message may be improved in the future with composition functions like: prefix, suffix, etc. There is already a composition validator called message that replaces the inner message with another one.
//(optional) Choose a value that is either a num in the set: (-10,5] U {7, 8} U (100, +inf) or an even integer
isOptional(or([
    isNum(or([
        and([gt(-10), ltE(5)]),
        equal(7),
        equal(8),
        gt(100),
    ])),
    isInt(
        (int value) =>value % 2 == 0 ? null : 'The input must be even.',
    ),
]));

@ArturAssisComp
Copy link
Contributor Author

I also explored possibilities using operator overloading which would allow things like: req() * isNum() * (gt(10) * lt(15) + gtE(20)), using boolean logic notation as example. But it would become very complex to handle some edge cases and the error message composition, and the compiler would not help too much during the composition. It would be necessary to rely on some runtime check or to maintain the generality of each validator, maintaining the redundancy.

Copy link
Contributor

@deandreamatias deandreamatias left a comment

Choose a reason for hiding this comment

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

Sorry for delay, I wanted to see this with time and in detail.

I really like this new propouse. I think that this is the way to foward.

@deandreamatias deandreamatias merged commit 7f0c09b into flutter-form-builder-ecosystem:refactor_2024_11 Feb 6, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API change suggestion]: Adding elementary validators
2 participants