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

Feature/#4 provide default checks #41

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Shahondin1624
Copy link

Providing a base implementation for #4

@effad
Copy link
Owner

effad commented Jun 1, 2024

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

* @return self for methodChaining
*/
public Check matchesRegexCheck(StringProperty property, Severity severity, Node decorated, String regex) {
Function<String, String> messageCreator = string -> String.format("%s['%s'] does not match the regex %s", property.getName(), string, regex);
Copy link
Owner

Choose a reason for hiding this comment

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

This is a problem for i18n. We must not provide any natural language strings from within ValidatorFX.

@effad
Copy link
Owner

effad commented Jun 17, 2024

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 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.

@Shahondin1624
Copy link
Author

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.

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.

None yet

2 participants