ValidatorFX icon indicating copy to clipboard operation
ValidatorFX copied to clipboard

Feature/#4 provide default checks

Open Shahondin1624 opened this issue 1 year ago • 3 comments

Providing a base implementation for https://github.com/effad/ValidatorFX/issues/4

Shahondin1624 avatar May 29 '24 16:05 Shahondin1624

Thank you for the PR. Please be patient, I'll try to look into it the week after the next.

effad avatar Jun 01 '24 07:06 effad

I had another look at this PR and I'm not sure if I want to integrate it the way it stands. The problems I found:

  • Messages must be i18n
    • If we just add messageCreator functions, the interface will have even more parameters
    • i18n can be done as parameter or by using a "string repository interface" ...
    • ValidatorFX until now elegantly sidestepped the problem by requiring the library client to provide message strings within the check-function
  • There's always (at least) two variants of create...Check: One with a node, another one without node. Keeping to the fluent API style of ValidatorFX, this should be done with a separate decorates(...) call, espcially since one check can also decorate multiple nodes.
  • Passing severity is contrary to what we do with "normal" checks, where severity is decided in the Consumer<Context> passed in withMethod
  • the isAssignableTo... checks confused me; they should be named isConvertibleTo... or isMappableTo
  • regular checks habe ObservableValue as the "lowest common denominator" of what can be observed. DefaultChecks uses Property and the name of the Property
    • I didn't even know properties can have a name
    • Most properties we get from JavaFX widgets have either nondescript names or no name at all

So overall I think I'll use this PR as a good base / inspiration of what is wanted but will try to come up with an implementation that is more in line with the (undocumented, implicit ...) design ideas behind ValidatorFX.

effad avatar Jun 17 '24 10:06 effad

I attempted to address your concerns regarding my pull request:

  • i18n: I tried to mimic what you did with the way decorations work, but I'm not entirely certain this is an optimal way.
  • decorates: I removed the overloaded methods that had the decorates parameter
  • Severity: Maybe this could be solved using a builder pattern, but I'm not really confident that this would provide a better developing experience
  • isAssignableTo: I renamed those methods to your proposed method name 'isMappableTo'
  • "lowes common denominator": I refactored the methods to use ObservableValue instead of properties. This also lead to introducing a new parameter "fieldName"

If you can spare the time, you could reevaluate whether the PR now fits your ideas behind ValidatorFX more closely, otherwise I think using the methods in this PR as inspiration for a future enhancement is fine as well.

Shahondin1624 avatar Jun 17 '24 20:06 Shahondin1624