- 
                Notifications
    You must be signed in to change notification settings 
- Fork 4
          Add allowString and unsigned options to integer assert
          #166
        
          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
base: master
Are you sure you want to change the base?
Conversation
0e9179a    to
    5dc5759      
    Compare
  
    5dc5759    to
    5181567      
    Compare
  
    allowString and unsigned options to integer assert
      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 think this PR solves some interesting problems of the Integer assert at the cost of a increased complexity and some decisions we need to make.
- 
Do we want to allow strings at all on this assert considering we're validating integers? Could be a contradictory statement, but not unlikely in javascript land. 
- 
The current regexp only accepts integers without leading zeros because a number like 01will be interpreted by javascript as1, so it wouldn't make sense to accept any other form. The new unsigned regexp, as it is, accepts any form of integer (with or without leading zero), so there is a duality in behaviour which will likely confuse developers.
- 
Would a separate assert make more sense? I.e. something in the line of Numeric?
|  | ||
| #### Arguments | ||
| - `allowString` (optional) - whether the assert accepts string inputs or not. | ||
| - `unsigned` (optional) - whether the assert should consider only unsigned values or not. | 
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.
Should accept?
| const int = /^(?:[-+]?(?:0|[1-9][0-9]*))$/; | ||
| const int = { | ||
| signed: /^(?:[-+]?(?:0|[1-9][0-9]*))$/, | ||
| unsigned: /^(?:\d+)$/ | 
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.
Keep same style? [0-9] instead of \d+?
| Tests if the value is an integer. | ||
|  | ||
| #### Arguments | ||
| - `allowString` (optional) - whether the assert accepts string inputs or not. | 
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.
Use strict instead?
This PR adds a few options to the
Integerassert:allowString: This option allows the assert to validate integers represented as strings.unsigned: This option forces only simple unsigned integers.The
questionhere is if we want to allowintegerassert to be a string and the impact for users confronted with anintegerassert that forces the input to be a string. Also it would be nice to come to a conclusion on distinguishing between integers with leading zeros or not and if an option should be presented for that matter.