Add "options" to describe output

[WesTyler]

Adds the options set by Joi.any().options({}) to the output from .describe(), per #1130

Old Behavior

Joi.any().options({ abortEarly: false, convert: false }).describe()
// { type: 'any' }

New Behavior

Joi.any().options({ abortEarly: false, convert: false }).describe()
// { type: 'any', options: { abortEarly: false, convert: false } }

[AdriVanHoudt]

aren't doing a double check on this._settings.hasOwnProperty? I assume this is to prevent setting the empty object should https://github.com/hapijs/joi/pull/1132/files#diff-834068731e2e6ef966ecfef994bbae5eR694 always be false?

[WesTyler]

Yes, you're right. This was the cleaner option I could come up with, since this._settings also contains ref info from the .when method.

The first hasOwnProperty check in the conditional will only fire off for the first option found (if any), and you are right - that was to prevent setting the empty object.

The second hasOwnProperty is to ensure that only the options specified here can get reported by describe. (Those specific options are referenced by the any.options docs here)

[Marsup]

Can you expand on which properties you are filtering with this specifically ? Maybe they shouldn't be there in the 1st place.

[Marsup]

@WesTyler ?

[WesTyler]

@Marsup sorry, crazy busy at work the last week.

.when is using this._settings to save a reference to the baseType here. That's the main issue.

The settings I want to preserve are set by .options here and by .strict here.

Now that I look at it more, I think I can remove the filtering and just check to make sure that baseType is not the only value and that it doesn't get set. I'll take a stab at that as soon as I get a chance.

[WesTyler]

Ok, just pushed a commit to (hopefully) clean up the logic per this discussion.

[WesTyler]

Cleaned up the baseType setting filter logic. Now instead of making duplicate Object.hasOwnProperty checks and doing a [].some on the expected options, the settings description option is only set if

1. There is more than one property on this._settings
2. The single property on this._settings is not baseType

Scenario 1 also explicitly filters out the baseType setting.

[WesTyler]

While this is still sitting here, I found a "quirk"(?) in the behavior that I'm not if I like or not.

On this branch, using the new describe output including the options:

Joi.object().keys({
    a: Joi.string()
}).options({ presence: 'required' }).describe();


/*
{ type: 'object',
  options: { presence: 'required' },
  children: {
    a: {
      type: 'string',
      invalids: [ '' ]
    }
  }
}
*/

Thoughts on having the options description applied only to the root parent? Contrast that with this behavior:

Joi.object().keys({
    a: Joi.string().required()
}).required().describe();


/*
{ type: 'object',
  flags: { presence: 'required' },
  children: {
    a: {
      type: 'string',
      flags: { presence: 'required' },
      invalids: [ '' ]
    }
  }
}
*/

I think I like this, because the options is set at the root level, while the flagsare set at each level; having that difference reflected in the describe output feels okay to me, just wanted to make sure that was understood before this PR is considered for merging.

[Marsup]

OK, had time to think about it peacefully. I think I have a better strategy, also fixing another potential issue you might not have seen yet :

1. do a very simple Hoek.clone(this._settings) on any.describe(), I think it's important we clone to avoid people messing with options from outside
2. take care of the eventual baseType in alernatives.describe() (already overloaded), by :
   1. removing it from the options
   2. setting it at the correct place in the description

Now the "correct place" is supposedly following the same logic as the validation, meaning that something (don't know what yet) should follow the last when, assuming that none of the whens has both then and otherwise defined.

Sounds like good enough directions to you ?

[Marsup]

And about the "quirk", options are passed all the way down, so we can either assume people will know that, or apply it, but descriptions will take a hit on performance.

[WesTyler]

Alright, I pushed a couple of updates per discussions:

1. Used a Hoek.clone on this._settings to prevent changes via description.
2. Moved this._settings.baseType to this._baseType.
   a. Added description.baseType to the alternatives description.
   b. Removed baseType filtering from the description.options logic.

For the quirk, I would rather not apply it all the way down exactly for the performance reason. I don't know that the extra visibility is valuable enough to justify a perf hit.

[Marsup]

Don't think you need a clone here, this has no effect on joi objects.

[Marsup]

Just base should be enough.

[Marsup]

So all this block can be shortened to description.options = Hoek.clone(this._settings); ?

[WesTyler]

Good call. This used to be much messier, so when I pulled baseType out of _settings I was happy to have removed a bunch of conditional logic and didn't even see that I could simplify it even more haha.

[WesTyler]

@Marsup anything else you're waiting on from me for this? No rush, just making sure I'm not holding anything up.

[lock]

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.