Skip to content

Support ISO 8601 duration as a string validator#1613

Closed
pettyalex wants to merge 2 commits intohapijs:masterfrom
pettyalex:ISO_8601_Duration
Closed

Support ISO 8601 duration as a string validator#1613
pettyalex wants to merge 2 commits intohapijs:masterfrom
pettyalex:ISO_8601_Duration

Conversation

@pettyalex
Copy link

This addresses #1612 by adding iso duration as a type of string validator. I'm not sure the best design to add it on the date validator (because it's not a date), so I didn't.

This also adds a reasonable amount of unit tests to cover it. Tests and linter pass

@pke
Copy link

pke commented Oct 29, 2018

Nice, exactly what I need :) Wanted to propose that myself. Thanks for writing it up. Lets hope it gets merged soon.

@pettyalex
Copy link
Author

@Marsup

What do we need to do to get this PR approved? Is there further work necessary? Tests are updated.

@pke
Copy link

pke commented Oct 31, 2018

Is there a way to extend joi via plugins maybe? So things like this could be released as joi-plugin-string-duration and would not depend on the maintainers of this repo to merge PRs that add functionality.

Works quite well with other open source libs (eslint, webpack, parcel etc)

A little hacky working approach would be:

// joi-string-duration/index.js
const Joi = require("joi")
const JoiErrors = require("joi/lib/language")

JoiErrors.errors.string.isoDuration = "must be a valid ISO 8601 duration"

Joi.string().__proto__.isoDuration = function () {
  return this._test('isoDuration', undefined, function (value, state, options) {
    if (/^P(?!$)(\d+Y)?(\d+M)?(\d+W)?(\d+D)?(T(?=\d)(\d+H)?(\d+M)?(\d+S)?)?$/.test(value)) {
      return value
    }
    return this.createError('string.isoDuration', { value }, state, options)
  })
}

@WesTyler
Copy link
Contributor

WesTyler commented Oct 31, 2018

Is there a way to extend joi via plugins maybe?

Yes! :) extensions

What do we need to do to get this PR approved?

I will try to review later today if nobody beats me to it.

@pke
Copy link

pke commented Oct 31, 2018

Ah cool, @WesTyler
I did not see that mentioned in the readme, where I only looked. Forgot to check the API.
So then this could be also implemented as a plugin. Good to know!
Even better if you can review it :)

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.

Only one comment, LGTM otherwise.

return internals.isoDate.test(value);
}

_isIsoDuration(value) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is it doing in this file if it's never going to be used here?

Copy link
Author

Choose a reason for hiding this comment

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

I didn't want to put the regex for ISO duration into the string index.js, but rather follow the existing pattern. I also don't want to reach into date's internals from the string validator.

I could just move this regex into the string validator, but I feel like it belongs next to the date regex.

Copy link
Collaborator

@Marsup Marsup Nov 29, 2018

Choose a reason for hiding this comment

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

Well, since it's never going to be involved in the creation of a date, I'm not so sure. It could even be in number for all I know. I don't usually use those, how are you going to consume that data?

@Marsup Marsup self-assigned this Nov 11, 2018
@Marsup Marsup added the feature New functionality or improvement label Nov 11, 2018
@Marsup
Copy link
Collaborator

Marsup commented Nov 11, 2018

Actually the documentation for the new method and the new error are missing as well, would be great if you could add that.

@Marsup Marsup force-pushed the master branch 2 times, most recently from 4a2de06 to a9aa3e7 Compare November 19, 2018 21:17
@hueniverse hueniverse added this to the 16.0.0 milestone May 29, 2019
@hueniverse hueniverse self-assigned this May 29, 2019
@hueniverse hueniverse added the v16 label Aug 10, 2019
@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.

Labels

feature New functionality or improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants