feat: invalidDefaults option to warn when defaults are ignored, fixes #957

[not-an-aardvark]

What issue does this pull request resolve?

#957

What changes did you make?

This adds an invalidDefaults option to implement the suggestion from #957.

(I chose the name invalidDefaults rather than ignoredDefaults because the defaults are no longer actually ignored, depending on the value of the option.)

Is there anything that requires more attention while reviewing?

I'm not very familiar with how the dotjs templates work, so it might be worth double-checking that I did that correctly.

[epoberezkin]

Thank you. Not convinced they should be called “invalid” :) They are valid from JSON Schema point of view, it’s just that Ajv ignores them as there is no easy way to apply them. Will review and merge the code over weekend - ok?

[not-an-aardvark]

Sure, no problem. Is there a name you would recommend instead? (Maybe something like unusedDefaults?)

[epoberezkin]

I think ignoredDefaults was fine, there are two options that allow value “ignore”. I am guessing here - maybe you think it looks too similar to “ignoreDefaults” and can be confusing? We also could “simplify” the whole thing and cram it into useDefaults option - “warn” would log unused defaults and “strict” would throw? Not sure :)

unusedDefaults is cool too - consistent with useDefaults

[epoberezkin]

unusedDefaults it is then :)

[not-an-aardvark]

I think my confusion with ignoredDefaults is that while the defaults were previously being ignored before adding this option, they are now sometimes logging a warning or reporting an error, which intuitively doesn't seem like they're being ignored

I'm certainly happy to change it to ignoredDefaults if you'd prefer -- I'm just mentioning this in case other people would be similarly confused.

[epoberezkin]

I think my confusion with ignoredDefaults is that while the defaults were previously being ignored before adding this option, they are now sometimes logging a warning or reporting an error, which intuitively doesn't seem like they're being ignored

I see... so we need some option that would mean to throw if some defaults are not used.

Maybe strictDefaults: true/'log'? The meaning is to only allow defaults that are used, and report other defaults?

From reading the issue you want to throw:

It should be possible to opt into a mode where invalid default keywords result in a compilation or validation error, rather than being ignored.

If so, we could do useDefaults: true/'strict' and leave any possible further extension to the future. Logging is not that useful normally...

Anyway, I am probably thinking too much about it :) Happy with either of the above: strictDefaults or useDefaults extension - your call.

[epoberezkin]

Going to merge it with strictDefaults - I want to release it today

[not-an-aardvark]

Thanks!