Allow validate options to be passed through assert/attempt #1722 #1723
Conversation
|
|
||
| const result = this.validate(value, schema); | ||
| const first = args[0]; | ||
| const message = ( |
There was a problem hiding this comment.
Wouldn't it be easier to just check if args[0] is an object?
Like:
const firstArgAsOptions = typeof args[0] === 'object';
const message = firstArgAsOptions ? null : args[0];
const options = firstArgAsOptions ? args[0] : args[1];There was a problem hiding this comment.
One of the possible values of message is a new Error instance, as new Error().
Unfortunately, that has the type 'object'
> typeof (new Error())
'object'| }; | ||
|
|
||
| root.attempt = function (value, schema, message) { | ||
| root.attempt = function (value, schema, ...args/* [message], [options]*/) { |
There was a problem hiding this comment.
One thought: how about making it an [message | options] like in string regex function? This would allow to use validate options AND the custom message. The third argument would be then either 'custom message' or { message: 'custom message', ...otherValidateOptions }.
What do you think about that?
There was a problem hiding this comment.
I implemented it in a way that message, options, both, or neither are acceptable. I even added a couple of tests around using both message and options in a few different scenarios.
As far as following the regex() param usage pattern, that's definitely an option. I was trying to keep the impact on current usage as minimal as possible, because attempt is actually used in quite a few places by internals behind the scenes.
There was a problem hiding this comment.
Yeah, I think we can keep this implementation. I just wanted to share my thought :)
| @@ -11,8 +11,8 @@ | |||
| - [`validate(value, schema, [options], [callback])`](#validatevalue-schema-options-callback) | |||
| - [`compile(schema)`](#compileschema) | |||
| - [`describe(schema)`](#describeschema) | |||
| - [`assert(value, schema, [message])`](#assertvalue-schema-message) | |||
| - [`attempt(value, schema, [message])`](#attemptvalue-schema-message) | |||
| - [`assert(value, schema, [message], [options])`](#assertvalue-schema-message-options) | |||
There was a problem hiding this comment.
I think this is not clear that options can come in place of message. There is a notation for this case but I can't recall it right now.
There was a problem hiding this comment.
I was just following the formatting for validate which has a similar flexible contract with options and callback both being optional.
There was a problem hiding this comment.
True. Then I guess that consistency is a better option here, let's leave it as is 😃
|
Btw. thanks for taking this! 😃 |
|
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. |
Per discussion in #1722
One note is that since
attempt/assertleverageerror.annotate(), the error message is messierthan I think @sarneeh was hoping for. This was already the case for
attempt/asserton objects, though, and allowingabortEarlythrough does add the additional field annotations as well as the additionalerror.details.