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

Boolean strict mode #5211 #5288

Closed
wants to merge 5 commits into from
Closed

Boolean strict mode #5211 #5288

wants to merge 5 commits into from

Conversation

c0d0g3n
Copy link
Contributor

@c0d0g3n c0d0g3n commented May 25, 2017

See #5211 and #4245. Strict mode (strict: true) throws cast error if input is not a boolean representation. Not backwards breaking as new behavior is opt-in via option. Unit test for accepted values included. Could not add documentation because make docs errors (should go into separate issue?).

@vkarpov15
Copy link
Collaborator

So first your branch is very confusing, hard to tell what actually changed. But looking at d57351e, I don't think I can merge this. strict: true is on by default, and already has its own semantics. I don't want to conflate new behavior with strict mode. I think a custom schema type or a plugin that replaces mongoose's Boolean.prototype.cast function would be more appropriate in this case.

@vkarpov15 vkarpov15 closed this May 26, 2017
@c0d0g3n
Copy link
Contributor Author

c0d0g3n commented May 26, 2017

Apologies, I indeed made a bit of a mess. For clarification, d57351e adds the strict option, ede9c64 adds a unit test. 8212b49 (getting up-to-date) was a pull/merge (I don't know anymore) from upstream, which in hindsight was probably not the way things go (Sorry, it's my first pr ever :)). I can create a new, proper pull request if you want.

You're probably referring to this: http://mongoosejs.com/docs/guide.html#strict? I thought it wouldn't be a problem because that option only applies to schemas, whereas the pr operates only on paths of type Boolean. How about changing strict in this pr to strictBool or strictCast to avoid confusion?

@vkarpov15
Copy link
Collaborator

I'm not very excited to add yet another option to schemas, that's fast becoming a dumping ground for 4.x behaviors that we want to get rid of. However, I can stomach it, strictBool it is :)

@c0d0g3n
Copy link
Contributor Author

c0d0g3n commented May 30, 2017

I understand that it would be more elegant to update behavior in 5.0, if that is what you mean. I've a workaround published here for now. I was also thinking if it would be useful that users can customize the values that are accepted as true and false, because in extreme cases users may not want to validate f.i. 'true' as true either. (Maybe we should even abolish converting altogether and introduce something like Wordpress filter hooks.) This may be a little far fetched though.

Anyway, just give me a shout if you still want a clean pr with strictBool and docs.

@vkarpov15
Copy link
Collaborator

Users can customize the values by overwriting the boolean type's cast() function, I don't see a need to add any more abstractions there currently. However, if you're still willing to do a clean strictBool PR I'd be happy to merge it 👍

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.

2 participants