Add use of hyphen or spaces in MACaddress #1065
Conversation
There was a problem hiding this comment.
I left some comments on my own work for discussion and also help guide fast reviews.
Cc. @profnandaa @chriso.
src/lib/isMACAddress.js
Outdated
| @@ -2,11 +2,13 @@ import assertString from './util/assertString'; | |||
|
|
|||
| const macAddress = /^([0-9a-fA-F][0-9a-fA-F]:){5}([0-9a-fA-F][0-9a-fA-F])$/; | |||
| const macAddressNoColons = /^([0-9a-fA-F]){12}$/; | |||
| const macAddressUsingHyphen = /^([0-9a-fA-F][0-9a-fA-F]-){5}([0-9a-fA-F][0-9a-fA-F])$/; | |||
| const macAddressUsingSpaces = /^([0-9a-fA-F][0-9a-fA-F]\s){5}([0-9a-fA-F][0-9a-fA-F])$/; | |||
There was a problem hiding this comment.
It is not a review of this work, but I tried doing:
/^([0-9a-fA-F][0-9a-fA-F](:|-)){5}([0-9a-fA-F][0-9a-fA-F])$/;This is to allow use of - or : has proposed in one regex. The issue is that, it made this valid:
01-02-03:04-05-ab, which is not.
Separating the regex allowed to validate different options. It is a convention to use either :, - or .
On that note, I also added spaces, which was not in the initial proposed bug. But, on research, it is still a valid MACAddress.
If there is a better way of making the regex one instead of 3 or 4 variables, I am welcoming ideas from the community. Thank you.
Cc. @profnandaa @chriso
There was a problem hiding this comment.
From what I see, the code looks okay, making it one regex will almost be the same logic since we will be OR'ing |).
Just a small nitpick, perhaps calling the variables ...With... instead of ..Using... will sound more "palatable" :)
Other than that, all LGTM 👍
There was a problem hiding this comment.
Good, making the changes and pushing them.
| @@ -2,11 +2,13 @@ import assertString from './util/assertString'; | |||
|
|
|||
| const macAddress = /^([0-9a-fA-F][0-9a-fA-F]:){5}([0-9a-fA-F][0-9a-fA-F])$/; | |||
| const macAddressNoColons = /^([0-9a-fA-F]){12}$/; | |||
| const macAddressUsingHyphen = /^([0-9a-fA-F][0-9a-fA-F]-){5}([0-9a-fA-F][0-9a-fA-F])$/; | |||
| const macAddressUsingSpaces = /^([0-9a-fA-F][0-9a-fA-F]\s){5}([0-9a-fA-F][0-9a-fA-F])$/; | |||
|
|
|||
| export default function isMACAddress(str, options) { | |||
| assertString(str); | |||
| if (options && options.no_colons) { | |||
There was a problem hiding this comment.
I was also still figuring out, why do we need an option for colons?
Instead of just allowing different cases of validations.
Cc. @profnandaa @chriso
There was a problem hiding this comment.
Sure, that might not be necessary.
There was a problem hiding this comment.
To make sure that I don't break on things, I will leave it for now.
And propose changes.
Thanks, @profnandaa.
|
Check on comments and replied to them. |
- Authoured_by: Ezrqn Kemboi <[email protected]> - Closes validatorjs#1061
|
@ezrqnkemboi - can you rebuild? |
|
Alright, working on that. |
Ref: #1061.