-
-
Notifications
You must be signed in to change notification settings - Fork 100
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
Date validator request #204
Comments
I think this would be a great idea! Would happily accept a PR! |
@snewcomer Done, let me know if it works for you. |
@snewcomer Thanks a lot for implementing the date validator in #218. Can't count how often I have implemented something like this in app space. I should have pushed one of them upstream. 🤦♂️ I'm wondering why you decided to not support strings. I often used a date validator directly together with
Would you accept a pull request implementing such a support? |
👋 Good question. So I thought it might be good to not allow strings for 2 reasons.
What are your thoughts? Is the current validator somewhat flexible enough for your use case? |
You could also see it the other way around: Information about the date is lost on conversion from ISO8601 string to a
In most cases the date should include time and time zone information but for some dates this conversion could be wrong. E.g. new year is independent of time zone. It was at Sometimes I prefer to not convert the ISO 8601 date strings to not loose that information.
Not sure about moment to be honest but for native
I think it's flexible enough but I don't like that I need to convert the date string to a |
I think you have brought up some good points. I would also note...
Either we convert it to a My concerns revolve around the variety of permutations the strings may come in as you have noted. The date from the input might be different string format than the date based to new Date('2020-01-01T10:00:00Z')
new Date('2020-01-01T10:00:00') Any other thoughts or ideas around supporting strings? |
I understand your concerns but to be honest I think most of them are caused by relying on I think this could be implemented safely without introducing performance issues if having three restrictions:
The first restriction ensures that we don't have to deal with date parsing at all. The second one limits the possible formats to a small set. The third one allows us to do a simple string comparison. If I didn't missed something the implementation should be straight forward. To verify that value and options are using the same format comparing the length of the strings should be enough. ISO 8601 spec ensures that two dates of the same format could be compared as strings. Not sure if it should also verify that it's a valid date string. E.g. either throw or fail validation if value is |
I could be on board with string comparison! And I agree |
Not limiting to a subset of ISO 8601 opens up a wide range of edge cases. The spec allows so much different ways to express the same information. Also I don't see a use case. Consumers either validated the return of a native input field or of a library. In the later case it's much likely that the library either returns a Not limiting to ISO 8601 would allow formats for which string comparison does not work. E.g. a consumer may use it with German locale format, which is What string formats do you like to support additionally? Edit: Just noticed that this may be caused by confusing the format returned by |
Ah I assumed the value when mutating an input[date] was not iso8601, but if it is, then 👍 . Would you be willing to put up a PR with a test / dummy example with the native input and we can start from there? |
@jelhan ping! Lmk if you would like me to take it on! I got some time this week. |
This would be awesome. I'm totally overloaded with Open Source stuff currently. 😢 |
It'd be great to allow the before, onOrBefore, after, onOrAfter options accept a function that returns a date. This way every time the validator is executed we can re-evaluate those values instead of evaluating only when the validation is defined. This is useful if we need to use a value that may change as the value for before, onOrBefore, after, onOrAfter (ex other date properties on the changeset). I'd be happy to PR this change if there is agreement here. |
@steveszc Make sense. But wondering if this is needed on all validators. Or is |
I think if we started with the |
@snewcomer just put up a PR adding the function support for date validator options. |
Thank you!! |
@jelhan Do you have an interest in taking a look at this PR? I did it sort of naively. But the plan would be to use this here if it fit your use case! |
This package is currently not providing a Date validator, which would be very useful for validating date-type inputs. Such a validator is already provided by
ember-validators
but is not in this project.https://github.com/offirgolan/ember-validators/blob/v2.0.0/addon/date.js
Having it available would allow us to use changesets for date inputs with something better than
which is currently the best, but somewhat hacky, implementation option.
The text was updated successfully, but these errors were encountered: