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.

Sign up for GitHub

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

Jump to bottom

Allow passing an option to string().isoDate() #1197

Merged
merged 1 commit into from
Jun 30, 2017
Merged

Allow passing an option to string().isoDate() #1197

merged 1 commit into from
Jun 30, 2017
+60 −30

Conversation

bdgamble
Copy link
Contributor

@bdgamble bdgamble commented May 23, 2017 •
edited by DavidTPate
Loading

Allow passing an option to string().isoDate() which formats the string as the simplified extended ISO format

Mimics the formatting returned by date().iso(), and the default ensures backwards compatibility.

ideally would use convert option to toggle whether the conversion happens, but since convert defaults to true, this would be a breaking change.

Closes #1196

@bdgamble bdgamble force-pushed the allow-format-string-isoDate branch 2 times, most recently from 06e46a9 to 847cb49 Compare May 23, 2017 21:51
@AdriVanHoudt
Copy link
Contributor

would it not make more sense to make it follow the convert option?

WesTyler
WesTyler suggested changes May 24, 2017
View reviewed changes
Copy link
Contributor

@WesTyler WesTyler left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I would actually rather this be a breaking change and follow the convert path instead of the options object, too.

Also there's a test failing in the Node v4 suite ;)

@bdgamble
Copy link
Contributor Author

Yah figured it should follow that option, but wanted to introduce it as a non breaking change first. I'll update and fix that test :)

@bdgamble bdgamble force-pushed the allow-format-string-isoDate branch 2 times, most recently from f7e9e5b to bcae592 Compare May 24, 2017 17:44
@DavidTPate DavidTPate changed the title Allow passing an option to string().isoDate() which formats the strin… Allow passing an option to string().isoDate() May 25, 2017
DavidTPate
DavidTPate suggested changes May 25, 2017
View reviewed changes
lib/types/string/index.js Outdated
try {
return new Date(value).toISOString();
}
catch (err) { }
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be handling the error here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I now return the isoDate Error in the catch block with some additional details (the thrown err which will say why it was unable to convert to ISO string, and the fact that convert is true) instead of just falling through to the standard isoDate error. Would you prefer I add a new error type (i.e. isoDateConvert) that has a message that explicitly calls out that it was the conversion that failed?

@bdgamble bdgamble force-pushed the allow-format-string-isoDate branch from bcae592 to 388688a Compare May 25, 2017 13:46
@AdriVanHoudt
Copy link
Contributor

Looking good!

Since convert defaults to true would this be a breaking change?
Also in the tests I would explicitly set convert to true. If the default changes it should not break the test imo

@bdgamble bdgamble force-pushed the allow-format-string-isoDate branch from 388688a to 258ad7a Compare May 25, 2017 14:58
@bdgamble
Copy link
Contributor Author

@AdriVanHoudt it would be a breaking change because now dates passed as 2017-01-01 to joi.string().isoDate() will be returned as 2017-01-01T00:00:00.000Z (in the same way joi.date.iso() does) instead of just the string being validated like before

@bdgamble
use the convert option to signal formatting isoDate strings in the si…
2a8f232 
…mplified extended ISO format

Mimics the formatting returned by date().iso()

Includes tests
@bdgamble bdgamble force-pushed the allow-format-string-isoDate branch from 258ad7a to 2a8f232 Compare May 26, 2017 14:45
@Marsup Marsup self-assigned this Jun 2, 2017
@Marsup Marsup added breaking changes Change that can breaking existing code bug Bug or defect labels Jun 2, 2017
Marsup
Marsup requested changes Jun 2, 2017
View reviewed changes
Copy link
Collaborator

@Marsup Marsup left a comment

Choose a reason for hiding this comment

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

I don't really mind this change personally, but it's a bit of a stretch. ISO format has many optional parts, and the dates you mentioned in the original issue are already in ISO format afaict, it's just not the format you'd want.

lib/types/string/index.js
return new Date(value).toISOString();
}
catch (err) {
return this.createError('string.isoDate', { value, reason: err, convert: options.convert }, state, options);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this line is needed, that context is wrong.

@Marsup Marsup added this to the 11.0.0 milestone Jun 15, 2017
@Marsup Marsup merged commit 8d682e7 into hapijs:master Jun 30, 2017
Marsup added a commit that referenced this pull request Jun 30, 2017
@Marsup
Cleanup for #1197
3bac3cb
@throrin19
Copy link

It's possible to set the convert false only for isoDate as isoDate option ? I just want to no convert that fields in my validate object

@lock
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Marsup Marsup Marsup requested changes

@DavidTPate DavidTPate DavidTPate requested changes

@WesTyler WesTyler WesTyler requested changes

Assignees
No one assigned
Labels
breaking changes Change that can breaking existing code bug Bug or defect
Projects
None yet
Milestone
11.0.0
Development

Successfully merging this pull request may close these issues.

string isoDate should allow the string to be converted to same format as date iso schema
6 participants
@bdgamble @AdriVanHoudt @throrin19 @Marsup @DavidTPate @WesTyler