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

Adding additional date validation options. #168

Conversation

aaronbhansen
Copy link
Contributor

I needed some additional date validation checks. Rather than write a custom validator, I figured I would create a pull request adding the functionality.

This adds two new date validation options:

beforeOrSame
afterOrSame

Also added a precision quantifier so that if you are passing full dates in from ember data or another source you could quantify what to compare on. For example, you could limit the precision to 'day' to ignore any time differences.

@offirgolan
Copy link
Collaborator

This is fantastic! The one thing I dont like is the OrSame postfix. What do you think of using OrOn instead?

  • beforeOrOn
  • afterOrOn

@aaronbhansen
Copy link
Contributor Author

I have no preference, I was just copying moment js method name format. I updated the code to OrOn format.

@offirgolan
Copy link
Collaborator

Okay you might hate be but can you change it to onOrBefore and onOrAfter? Ill merge after. Just think it sounds better tbh. 😸

@aaronbhansen
Copy link
Contributor Author

I can change it, one thing I did like about beforeOrOn / afterOrOn, is that if your validators were previously before / after, it was less work / error prone to change to beforeOrOn or afterOrOn by just appending onto the end. If you still want it changed after that input I'll do it. Let me know.

@offirgolan
Copy link
Collaborator

True, but doing a simple find/replace from before to onOrBefore is not too hard 😜. Honestly, its more of it will bother me later if we dont do it now kinda thing lol

@aaronbhansen
Copy link
Contributor Author

Its changed

@offirgolan
Copy link
Collaborator

👍 Thanks for dealing with my antics! Ill merge once the build passes.

@aaronbhansen
Copy link
Contributor Author

No problem. Glad to help out

@offirgolan
Copy link
Collaborator

Im sorry, just for clarification, is the default precision in moment seconds? If you just pass moment().isAfter(date) is the precision assumed to be in seconds?

@aaronbhansen
Copy link
Contributor Author

In moment js the default is milliseconds as seen here We could add that option, but it seemed far more granular than a validation would ever need.

@offirgolan
Copy link
Collaborator

I don't think that we should actually default the precision. I'm assuming
that if you pass undefined, it should just use moment's default.

On Saturday, April 16, 2016, Aaron Hansen notifications@github.com wrote:

In moment js the default is milliseconds as seen here
https://github.com/moment/moment/blob/2749f08a24056f93dd2208f2744f21b35f6cd999/src/lib/moment/compare.js#L11
We could add that option, but it seemed far more granular than a validation
would ever need.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#168 (comment)

Thanks,
Offir Golan

@aaronbhansen
Copy link
Contributor Author

I removed the default precision. I didn't list milliseconds as an option, since thats what it internally defaults to anyway if we don't pass it in.

@@ -53,9 +59,11 @@ export default Base.extend({
},

validate(value, options) {
options = assign({}, options);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line isn't needed here because the options object is already a new pojo created from processOptions method. You can also remove the const assign declaration from the top.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep your right, I thought it shouldn't need it. The tests are mutating the options because processOptions isn't called. I need to clone the object in the tests. I'll get that fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the changes, let me know if you want anything else done.

@offirgolan
Copy link
Collaborator

👍 Thanks for taking the time to make all those changes @aaronbhansen!

@offirgolan offirgolan merged commit d39f258 into adopted-ember-addons:master Apr 18, 2016
@aaronbhansen
Copy link
Contributor Author

No problem, glad to help out. Next pull request I'll know what you're looking for more 😄

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