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

Add startsWith and endsWith for URLs. #6

Merged
merged 4 commits into from Dec 22, 2014

Conversation

@antony
Copy link

antony commented Nov 4, 2014

I realise that this library is intended to be as small and light as possible - but I think these two assertions are essential for assertions on urls - redirects - domains etc, especially when considering the code+lab marriage.

Include goes part of the way, but checking a relative url when given an absolute url and checking schemes on domains are something I end up doing every day, hence this PR.

  • I say URLs but there are lots of other uses (titles in html for example often share a common prefix). Let me know if there is anything I've missed.
@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Nov 4, 2014

You can do this now with expect('abc').to.match(/^a/) or expect('abc').to.match(/b$/). I'll think about it.

@hueniverse hueniverse added the feature label Nov 4, 2014
@hueniverse hueniverse self-assigned this Nov 4, 2014
@antony

This comment has been minimized.

Copy link
Author

antony commented Nov 4, 2014

Ah yes - true, but I try to keep RegExp out of my tests for clarity. Thanks for considering :)

README.md Outdated
@@ -31,6 +31,8 @@ Lead Maintainer - [Eran Hammer](https://github.com/hueniverse)
- [`null()`](#null)
- [`undefined()`](#undefined)
- [`include(values)`](#includevalues)
- [`startWith(value)`](#startWith)

This comment has been minimized.

Copy link
@cjihrig

cjihrig Nov 25, 2014

Contributor

The link target should be #startwithvalue.

README.md Outdated
@@ -31,6 +31,8 @@ Lead Maintainer - [Eran Hammer](https://github.com/hueniverse)
- [`null()`](#null)
- [`undefined()`](#undefined)
- [`include(values)`](#includevalues)
- [`startWith(value)`](#startWith)
- [`endWith(value)`](#endWith)

This comment has been minimized.

Copy link
@cjihrig

cjihrig Nov 25, 2014

Contributor

The link target should be #endwithvalue.


internals.addMethod(['startWith', 'startsWith'], function (value) {

var comparator = this._ref.slice(0, value.length);

This comment has been minimized.

Copy link
@cjihrig

cjihrig Nov 25, 2014

Contributor

Should you assert that the data type is a string or possibly perform a coercion before calling slice()

@@ -711,6 +711,49 @@ describe('expect()', function () {
});
});

describe('endWith()', function () {

This comment has been minimized.

Copy link
@cjihrig

cjihrig Nov 25, 2014

Contributor

Should probably add some tests for other data types.

@hueniverse hueniverse assigned cjihrig and unassigned hueniverse Nov 26, 2014
@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Nov 26, 2014

@cjihrig all yours now...

@antony

This comment has been minimized.

Copy link
Author

antony commented Nov 27, 2014

Thanks @cjihrig and @hueniverse - I'll get on these fixes asap.

@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Dec 5, 2014

@antony any update on this?

@antony

This comment has been minimized.

Copy link
Author

antony commented Dec 11, 2014

@cjihrig Sorry - absolutely snowed under at the moment with my startup - i'll sync with upstream as soon as I get a free minute and make the changes. Sorry about the delay!

@antony

This comment has been minimized.

Copy link
Author

antony commented Dec 15, 2014

Hi @cjihrig - grabbed a couple of minutes to go through your suggestions. I've restricted the assertion to strings only - I toyed with arrays for a while but I couldn't think of a decent use case where you wouldn't assert more explicitly so I've gone with a string-only assertion.

Please let me know if there are any further suggestions.


internals.assert(this, typeof this._ref === 'string' && typeof value === 'string', 'Can only assert endsWith on a string, with a string');

var comparator = this._ref.slice(-Math.abs(value.length));

This comment has been minimized.

Copy link
@arb

arb Dec 15, 2014

Contributor

Is Math.abs necessary in this context? It does add some overhead and I don't think we really need it.

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Dec 15, 2014

You should probably note that it's case sensitive since we are just using simple === and no regular expressions.

… index before negating
@antony

This comment has been minimized.

Copy link
Author

antony commented Dec 21, 2014

@arb I've updated the PR as per your suggestions, Thanks!

@arb arb added this to the 1.3.0 milestone Dec 22, 2014
arb added a commit that referenced this pull request Dec 22, 2014
Add startsWith and endsWith for URLs.
@arb arb merged commit 6c43718 into hapijs:master Dec 22, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.