search

o-d-i-n / is-alpha

MIT License
5 stars 0 forks source link

patternadded #2

Closed himanshu81494 closed 8 years ago

himanshu81494 commented 8 years ago

some patterns added

mbad0la commented 8 years ago

@bogas04 @amaneureka @karanchahal please review.
@himanshu81494 thanks for the pull request. Will merge it after the code has been reviewed by atleast one of them.

mbad0la commented 8 years ago

@himanshu81494 also as a good practice, always drop in a comment on an existing issue or create a new one when you are looking to contribute some code. That helps us to have a discussion about it and increase the chance of having a relevant pull request.

karanchahal commented 8 years ago

Looks good @mbad0la , I think you should get a second opinion with @bogas04 though as you guys were the one who actually made the rep. But this should be okay barring a few things. One could make it a bit cleaner I suppose especially the isvalidIP section

mbad0la commented 8 years ago 
But this should be okay barring a few things.

@karanchahal please let me know your concerns with this. I'm concerned about the validity of the Regex used for testing the added cases.
@himanshu81494 can you refer us to some link(s) that specifies the conventions for writing down an IP address, host or MD5?

mbad0la commented 8 years ago

And if we do merge this, we'd have to resolve #3 first.

bogas04 commented 8 years ago

Thanks for the PR ! 👯

However, is IP thingy in scope of is-alpha ? I don't want it to end up becoming a repo of all sorts of regex.

What do you think @mbad0la ?

mbad0la commented 8 years ago

@bogas04 that's why I wanted your opinion about this. Having too many vague methods might cause confusion as well. As @karanchahal pointed out in #3 we can have email validation as it does come under string validation in some way.
What do you say? Should we merge this or we can maybe change these methods with some which are specific to this package?

bogas04 commented 8 years ago

As a developer, I won't trust is-alpha for giving nicer email validation regex over say is-email, so I think we shouldn't do more than required. is-alpha itself is a micro module, if we really want is-email, we can probably create another micro module for the same.

That's how I look at it.

mbad0la commented 8 years ago

@bogas04 alright.
@himanshu81494 sorry but this PR doesn't align with what we want this package to be. I hope you understand...