Skip to content
This repository has been archived by the owner on Dec 19, 2022. It is now read-only.

Proper check for value of min and max #21

Merged
merged 2 commits into from
Mar 19, 2015
Merged

Conversation

mehdivk
Copy link
Contributor

@mehdivk mehdivk commented Mar 18, 2015

This PR addresses this bug #20

@@ -21,13 +23,13 @@ module.exports = function (opts) {
}

if (opts) {
if (opts.min && opts.max && (value < opts.min || value > opts.max)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Small optimisation but should we do

    if (opts) {
      var hasMinValue = hasValue(opts.min);
      var hasMaxValue = hasValue(opts.max);
      if ( hasMinValue && hasMaxValue && (value < opts.min || value > opts.max)) {
        return 'should be a number between ' + opts.min + ' and ' + opts.max;
      }
      if (hasMinValue && value < opts.min) {
        return 'should be a number >= ' + opts.min;
      }
      if (hasMaxValue && value > opts.max) {
        return 'should be a number <= ' + opts.max;
      }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nguyenchr Nice optimisation.

@rprieto
Copy link
Contributor

rprieto commented Mar 18, 2015

Nice catch! 👍 I think we might have similar bugs in array and string, should we have a look as part of this PR?

On top of this, maybe we could add some checks on opts before returning the matcher, for example asserting that opts.min == null || typeof(opts.min) === 'number'. This would not have any runtime performance if it's executed outside of the matcher function we return. See the duration matcher, that asserts that the min and max durations are actually valid.

@mehdivk
Copy link
Contributor Author

mehdivk commented Mar 18, 2015

@nguyenchr Your suggestion is applied.
@rprieto Agree, I applied same pattern of validation to the number matcher.

@nguyenchr
Copy link
Contributor

nice one 👍

is it worth making the same change to array and string matchers in this PR?

@mehdivk
Copy link
Contributor Author

mehdivk commented Mar 18, 2015

@rprieto @nguyenchr Good idea. I will extend this PR to fix other matchers as well.

@mehdivk
Copy link
Contributor Author

mehdivk commented Mar 19, 2015

@rprieto @nguyenchr Actually I don't think using this new method in array or string is a good thing. For it doesn't make sense if somebody says minimum length of array should be 0 or maximum length of array should be 0. It's same thing for string. I feel we should leave them as it's now. Putting new logic doesn't change anything in mentioned matchers, it only brings more process to the matcher.

Proper validation of min and max

optimization

min=0 or max=0 bug fix for Integer matcher.

Refacotring integer and number matchers.

Refactoring number matcher
@rprieto
Copy link
Contributor

rprieto commented Mar 19, 2015

Good point!

@nguyenchr
Copy link
Contributor

👍

Better formatting for title of releases
@mehdivk
Copy link
Contributor Author

mehdivk commented Mar 19, 2015

Per chat with @nguyenchr changelog.md is added to library to keep track of changelog for each version.

mehdivk pushed a commit that referenced this pull request Mar 19, 2015
Bug fix in validation of data via `integer` and `number` matchers when `{min: 0}` or `{max: 0}`
@mehdivk mehdivk merged commit 02b4314 into Tabcorp:master Mar 19, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants