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

[form.rule] Rename empty rule to be consistent #2937

Closed
mvorisek opened this issue Nov 17, 2023 · 9 comments
Closed

[form.rule] Rename empty rule to be consistent #2937

mvorisek opened this issue Nov 17, 2023 · 9 comments
Assignees
Labels
tag/next-release/nightly Any issue which has a corresponding PR which has been merged and is available in the nightly build type/feat Any feature requests or improvements
Milestone

Comments

@mvorisek
Copy link
Contributor

Feature Request

originally posted in atk4/ui#2129

Currently, the empty form rule is very, very confusing as the logic is negated with the remaining names.

empty means "nothing", but the rule validates the form field is "filled / not empty".

here atk4/ui#2129 (comment) is this fact more evident when compared with other rules like checked or is[dog]

This is a feature request to rename empty form rule to something like notEmpty or mandatory. For BC, the empty can be kept with a console warning.

@mvorisek mvorisek added state/awaiting-triage Any issues or pull requests which haven't yet been triaged type/feat Any feature requests or improvements labels Nov 17, 2023
@lubber-de lubber-de added this to the 2.10.x milestone Nov 17, 2023
@lubber-de lubber-de removed the state/awaiting-triage Any issues or pull requests which haven't yet been triaged label Nov 17, 2023
@mvorisek mvorisek changed the title [form.rule] Rename empty rule to notEmpty to be consistent [form.rule] Rename empty rule to be consistent Nov 24, 2023
@mvorisek
Copy link
Contributor Author

I would be happy if the better named empty rule can be implemented in v2.9.x. The empty rule can be deprecated with a console warning (or whatever the minimal default verbosity level it). In v2.10.x, it can be removed or kept until v3.

@lubber-de
Copy link
Member

Ok, i am preparing a PR. I thought about naming the new rule required instead of notEmpty?
This would at least fit the existing "autocheckRequired" functionality

Opinions?

@lubber-de
Copy link
Member

Mmh, notEmpty would indeed fit better in the same terms as all the other rules, as the validation checks for "not empty" and not "if the field was required"....

@jamessampford
Copy link
Contributor

Perhaps required should be added to ensure that there is a value of sorts (even a space)

notEmpty on the other hand could check that there is something useful there, but would need to trim the value to make sure that it isn’t just white space (space, tab, new line, etc)

There could possibly be use case where one needs to check that a field is empty with no value/whitespace. Such as do not complete this field if entered something elsewhere

@mvorisek
Copy link
Contributor Author

mvorisek commented Nov 28, 2023

Good points. I personally would prefer one function/name for "the 99% usecases", ie. a function that will check if the length is not zero after whitespaces are trimmed from the ends.

The reason I do not support additional function to check if an input is:

  • strictly empty without trimming
  • or "value is empty" like '0'

is that the first is not much useful in practice and the second is hard to define and basically any app needs a different criateria, like for some apps -1 is empty too. So user should define custom rule for these scenarious.

About the name, required or notEmpty LGTM, I have no preference.

@daveharris
Copy link

I've often wondered about this same issue, and came across this looking for something else. The negation issue is most-striking when sitting alongside other validations:

fields: {
  'host[first_name]': {
    rules: [
      { type: 'empty' },
      { type: 'maxLength[50]' },
    ]
  }

It appears the value needs to empty and less than 50 characters.

As for the name, coming from a Rails background, the presence validator has always made sense to me, so present would be my vote:

validates :first_name, presence: true, length: { maximum: 50 }

@mvorisek
Copy link
Contributor Author

Thank you for supporting this issue, but present is NOT name I support nor frequently used by any common core, especially string, API.

@lubber-de lubber-de self-assigned this Nov 30, 2023
@lubber-de lubber-de modified the milestones: 2.10.x, 2.9.4 Nov 30, 2023
@lubber-de lubber-de added the state/has-pr An issue which has a related PR open label Nov 30, 2023
@lubber-de
Copy link
Member

lubber-de commented Nov 30, 2023

I prepared that by #2949
Please take a look 😉

Edit: The trimming functionality is already inbuild via various possible "shouldTrim" setting (which is true by default)

@lubber-de lubber-de added tag/next-release/nightly Any issue which has a corresponding PR which has been merged and is available in the nightly build and removed state/has-pr An issue which has a related PR open labels Dec 2, 2023
@mvorisek
Copy link
Contributor Author

mvorisek commented Dec 2, 2023

Thank you all!

@mvorisek mvorisek closed this as completed Dec 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tag/next-release/nightly Any issue which has a corresponding PR which has been merged and is available in the nightly build type/feat Any feature requests or improvements
Projects
None yet
Development

No branches or pull requests

4 participants