-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add IP Address Validation and Separate IP and URI Logic to Files #619
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
Conversation
@Marsup I got them split out where I thought they should be. I left the validation of options within |
@@ -116,6 +116,9 @@ exports.errors = { | |||
uppercase: 'must only contain uppercase characters', | |||
trim: 'must not have leading or trailing whitespace', | |||
creditCard: 'must be a credit card', | |||
ref: 'references "{{ref}}" which is not a number' | |||
ref: 'references "{{ref}}" which is not a number', | |||
ip: 'must be a valid ip address', |
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.
Regroup those under one option.
ipOptions.version = [ipOptions.version]; | ||
} | ||
|
||
var version; |
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.
You don't need that outside the loop.
You're missing many line breaks in the tests as well but I won't comment on all of them. |
Gotcha. I'll go through on the pieces you commented on, as well as the rest of the code. I've been getting more familiar with the coding style from the |
@Marsup I think it's all set now. I went through and tried to find all the syntax issues I could and addressed your feedback. I also figured out what I did wrong when trying to squash things before (I wasn't force pushing before), so I squashed the changes together. |
|
||
var regex = internals.ipRegex; | ||
ipOptions = ipOptions || {}; | ||
Hoek.assert(typeof ipOptions === 'object', 'ipOptions must be an object'); |
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.
simply options
}); | ||
|
||
describe('#ip({ version: "ipvfuture" })', function() { | ||
it('should validate all ipvfuture addresses with a default CIDR strategy', function(done) { |
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.
Line break
Do you know how to rebase your commits or should I do it ? |
@Marsup It should be all squashed and include the changes for your comments about the error messages. |
Your patch contains things that have nothing to do with this, you must have failed the rebase. |
Darn! Must have been that merge previously to catch up to the 6.1.0 branch. I'll take a look. |
@DavidTPate where are we on this ? |
…ure with or without a CIDR. Separate out IP and URI as well as their shared parts into their own files to keep `string` simpler and easier to understand.
@Marsup The changes only include those for the PR now. That was really odd, I had changed permissions and items that I didn't expect included in my commits from when I merged somehow. Never had that happen before. |
Rebased on branch, thanks. |
This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions. |
This PR adds the ability to do IP address validation and updates the handling of the components used to build Regular Expressions for both IP and URI validation into files. Additionally, this replaces the old usage of
Array.reduce()
which was occurring before in both IP and URI validation construction in favor of the standardfor(...)
loop.This resolves #607 and #617