Skip to content
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

Fixed default message formatter (closes #41) #42

Merged
merged 4 commits into from
Aug 10, 2017
Merged

Fixed default message formatter (closes #41) #42

merged 4 commits into from
Aug 10, 2017

Conversation

niftylettuce
Copy link
Contributor

No description provided.

@matteodelabre
Copy link
Owner

Thanks for your help! As I understand this, it enables users to override the default message, something impossible when you use mongoose.Error.messages.general.unique? However, I can’t see the use case for having two ways of overriding the default (in the Schema and in the options). Do you have an example where this is useful?

@niftylettuce
Copy link
Contributor Author

niftylettuce commented Aug 8, 2017 via email

@niftylettuce
Copy link
Contributor Author

@matteodelabre can you take a look at this? ty

Copy link
Owner

@matteodelabre matteodelabre left a comment

Choose a reason for hiding this comment

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

Sorry for the delay! I think a few minor problems should be addressed first. Tell me what you think.

index.js Outdated
var tree = schema.tree, key, messages = {};

options = options || {};
if (!options.message) {
options.message = 'Path `{PATH}` ({VALUE}) is not unique.';
Copy link
Owner

Choose a reason for hiding this comment

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

To avoid confusion between the different ways to provide a custom error message, I suggest using options.defaultMessage instead of options.message.

index.js Outdated
* @return {Promise.<ValidationError>} Beautified error message
*/
function beautify(error, collection, values, messages) {
function beautify(error, collection, values, messages, message) {
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should rename message to defaultMessage to avoid confusion between the singular message and the plural messages variable.

README.md Outdated

The default value of `mongoose.Error.messages.general.unique` is ``"Path `{PATH}` ({VALUE}) is not unique."`` which adheres to [the Mongoose error message defaults](https://github.com/Automattic/mongoose/blob/master/lib/error/messages.js).
The default value of `options.message` is ``"Path `{PATH}` ({VALUE}) is not unique."`` which adheres to [the Mongoose error message defaults](https://github.com/Automattic/mongoose/blob/master/lib/error/messages.js).
Copy link
Owner

Choose a reason for hiding this comment

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

If we are going to introduce more API surface, I think we should be extra-clear on when one mechanism for overriding the message should be used instead of the other one. Currently, I have the feeling that options.message and the custom message in the unique field might feel redundant to the users.

In my opinion, the main use for options.message is for when the plugin is used globally. If you want to, I can try to improve the docs after merging this PR. I think renaming options.message to options.defaultMessage will make things clearer.

@niftylettuce
Copy link
Contributor Author

@matteodelabre if I change options.message to options.defaultMessage, then we exceed the 100 character limit for the line here #42 (comment). Also, I think that specifying the prefix default is a bit redundant considering these are default options. Can we get this merged as-is and released? You could always change later to whatever you'd like + update docs. I just really need this and would rather not use my fork in my package.json.

@matteodelabre
Copy link
Owner

matteodelabre commented Aug 10, 2017

We can always wrap that line if it becomes too long. I don’t think it’s clear enough for users that these are default options. No worries, I will make those changes by myself! :)

@matteodelabre
Copy link
Owner

What do you think?

@niftylettuce
Copy link
Contributor Author

Looks good to me

@matteodelabre matteodelabre merged commit 0d68bd2 into matteodelabre:master Aug 10, 2017
matteodelabre added a commit that referenced this pull request Aug 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants