Skip to content
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

Expose matcher regex #17

Closed
wants to merge 1 commit into from
Closed

Conversation

jacktuck
Copy link

@jacktuck jacktuck commented Feb 26, 2018

Tests pass locally if run with mocha

@davisjam
Copy link
Contributor

I'm curious -- why is this desirable? It breaks encapsulation, so if a more complex scheme were ever used to validate URLs (as in #18) there would be backwards compatibility issues.

@jacktuck
Copy link
Author

jacktuck commented Mar 20, 2018

@davisjam It may not be widely desirable. Although, i'm not quite sure i'm on the same page on compatibility, i'd have thought the situation would be the same given a regex change; it'd be a semvar major. My use case was, I was looking for a well tested regex packaged into a tidy module; but i don't want to directly call test on the regex. Instead, i wanted to use it with Joi's validation library.

const isUrl = require('is-url')

joi.object().keys({
  url: joi.string().regex(isUrl.matcher).required()
})

@davisjam
Copy link
Contributor

If the regex is not exported, then you could make a non-breaking change in how the URL is tested without changing the semver major.

That's what I've attempted (I think successfully?) in #18.

If the regex is exported, then any change to how the URL test is performed would be semver-major.

@jacktuck
Copy link
Author

jacktuck commented Mar 20, 2018

@davisjam Ah i got you. I think you can treat semvar exactly the same exposed or not; in any case the it's the behaviour of the regex that's depended on right and not it's exact pattern since two patterns can be equivocal, but different, for instance. In any case, this PR is not too important to me, just thought it would be a nice addition. So, happy to close for now. If this ever gets traction, would be happy to see it reopened and make any amends.

@jacktuck jacktuck closed this Mar 20, 2018
@davisjam
Copy link
Contributor

davisjam commented Mar 20, 2018

In #18 multiple regexes are used, and in futures perhaps still wilder changes might be afoot!

@jacktuck
Copy link
Author

@davisjam Fair enough :) Indeed, multiple regexes would make this untenable in any case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants