number: fix behavior of allowNone

[bgentry]

For the number validator, the docs said that when allowNone was true, validation would be skipped if the value was null or undefined. What was actually happening is that validation would not be skipped at all in those cases.

Instead, only if allowNone was false, the validation would fail if the value was null/undefined.

The smallest possible fix here included also removing the default value on allowNone. This turns it into a purely opt-in option. However, it seems likely this would cause less breakage than the alternative (simply fixing the behavior to match the documentation).

If you would prefer that I leave the default & documentation untouched and instead just make the behavior actually match the docs, let me know and I'll be happy to make that change.

Cheers :v:

[offirgolan]

@bgentry thanks for the fix! The one issue I have is that setting the default value to false is a breaking change and will require a major version release. Can you revert that?

[bgentry]

@offirgolan I think at least one of us is misunderstanding what this change will do. I attempted to preserve the current default behavior as best I could.

Previously, allowNone would default to true, which would just mean that the code would skip over the isNone check.

Now, you have to explicitly set it to true to get it to check isNone and bail out on validations. In both cases, if you don't manually specify this option, the validations will continue. Only those who have already specified true will see a change in behavior here, and that change will be that the behavior actually does what the docs say whereas it didn't previously.

Does that sound like the appropriate change to make here, or did I misunderstand my own code change? Can you be more specific on how you'd like it modified if at all?

[bgentry]

@offirgolan I have reinstated the default allowNone = true setting. Now, this change just makes the behavior match the documentation (which may surprise users who were relying on the old broken implementation).

[offirgolan]

@bgentry thank you. I'll make sure to document that in the release. I added 1 more comment that will clean up the code a little bit more 😄

[bgentry]

Made that change and fixed the broken test (which was previously verifying the old broken behavior).

[offirgolan]

@bgentry looks great! Thank you! 😸

[bgentry]

Hi @offirgolan, any chance you can cut a new release that includes this? 🙏

[offirgolan]

Sorry for the delay @bgentry! Released as v1.2.0.

[andreyfel]

@offirgolan it appears that ember-cp-validations v3.x doesn't contain this fix. So, when migrating from 3.x to 4.x you get this breaking change. So, I would suggest documenting this breaking change in Upgrading documentation https://github.com/offirgolan/ember-cp-validations/blob/master/UPGRADING.md