-
Notifications
You must be signed in to change notification settings - Fork 95
Add feature to allow personalised error messages per field. #57
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.
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
Conversation
icebob
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank @ispyinternet, great PR, but this solution causes performance degradation. I think there is no reason to manage custom messages in validation time because it can be managed in compilation time and performance won't be affected. What do you think? Could you modify the PR?
|
@icebob I've tried to adapt. Please review. |
lib/validator.js
Outdated
| messages[schemaType.type] = this.messages[schemaType.type]; | ||
|
|
||
| Object.keys(schemaType).forEach(rule => { | ||
| if (rule == "type" || rule == "items" || rule == "props" || rule == "messages") return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain what is this code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to remove check from run time we have to prepare all possible message types at compile time. These include:
‘required’
‘string’ | ‘number’ | ‘array’ etc (i.e type)
Then we analyse the additional rules to see what other messages need to be added. When analysing the rule object, obviously we ignore the other properties that we know are not additional rules, i.e. 'type', 'items' for arrays, 'props' for objects and our new 'messages' property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just changed the if statement to use key in obj style rather than truthy test
|
Good job, the performance has been improved! On my PC 5.3M ops/sec. The master branch is 5.2M 👍 |
|
please don't force push commit because i'm trying to linting the codes. |
|
sorry, I wont anymore |
icebob
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! So is it done? Can I merge?
|
All tests are passing, not sure if you want to review performance or do any further sanity checks. Some unexpected changes to review. |
|
The Master: |
|
Also for every rule we are now iterating over all the default messages. Previously only iterates if user supplied. Perhaps an overwriting merge of the two objects. I’ll make the update. Is there a command to run so I can get the metrics to report back? |
|
I've added a condition, so only handle custom messages if there is |
|
Now it's much better. √ compile & validate 119,438 rps By the way, you can check perf with |
|
Nice! Why did you remove Object.keys(this.messages) from constructor? Now it is called everytime a message property is encountered - negligible admittedly!? Also did you see the slight changes to |
|
Because there is not relevant difference. With Object.keys ~68k, with static array ~70k. But I'm checking again. In string & email files I made some linting in regexp. |
|
@ispyinternet I put back the key array. It gives +10%. So now I think it's ready & performant. Could you add this new feature to the README too with examples? |
|
I already updated the README its the last section |
|
Really, sorry :) |
|
Thank you for your work. |
|
Thanks for your help too! |
|
you can close #28 |
I've added tests and updated the readme if you are happy to include this feature.