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

include/string/contain doc errors #15

Closed
jfirebaugh opened this issue Feb 13, 2012 · 5 comments
Closed

include/string/contain doc errors #15

jfirebaugh opened this issue Feb 13, 2012 · 5 comments
Milestone

Comments

@jfirebaugh
Copy link
Member

The include API doc has this example

expect([1,2,3]).to.contain(2);

I think that's meant to be:

expect([1,2,3]).to.include(2);

string has this example:

expect('foobar').to.include.string('bar');

I think that's meant to be:

expect('foobar').to.have.string('bar');

Though, if you did want include to work as both a function and a chaining property, it seems to be possible:

Object.defineProperty(Assertion.prototype, 'include', {
  get: function () {
    var include = function (obj) {
      this.assert(
          ~this.obj.indexOf(obj)
        , 'expected ' + this.inspect + ' to include ' + inspect(obj)
        , 'expected ' + this.inspect + ' to not include ' + inspect(obj));

      return this;
    };
    include.__proto__ = this;
    return include;
  }
});

I think it would be nice if both include and contain worked this way (i.e. they were synonyms, with the definition above):

expect([1,2,3]).to.contain(2);
expect([1,2,3]).to.include(2);
expect('foobar').to.contain('bar');
expect('foobar').to.include('bar');
expect({foo: 1, bar: 2}).to.contain.keys('foo', 'bar');
expect({foo: 1, bar: 2}).to.include.keys('foo', 'bar');

I can send a pull request if you agree.

@logicalparadox
Copy link
Member

One thing to note is that keys behaves differently depending on the modifier..

expect({ foo: 1, bar: 2 }).to.have.keys('foo', 'bar'); // this is strict; all or nothing
expect({ foo: 1, bar: 2 }).to.contain.keys('foo'); // this is inclusion

This is achieved through this.contains, which can be seen triggered here and used here.

Provided that the strict/inclusion feature for keys is not lost, I have no objections. Also, if you would be so kind to include some tests, that would be awesome :)

logicalparadox added a commit that referenced this issue Feb 23, 2012
@logicalparadox
Copy link
Member

I fixed the small typos for the docs.

Is the second part, the feature request, something that you have had a chance to look into? It not, I'll slate it for 0.4.x.

@jfirebaugh
Copy link
Member Author

I ended up using similar ideas in chai-jquery, but haven't had time to do it for chai itself. Feel free to put it on the back burner.

@logicalparadox
Copy link
Member

i like the idea so im going to leave this open so it doesn't fall into the abyss of closed tickets

@logicalparadox
Copy link
Member

This has been implemented in the 0.6.x branch and will be in the next release :)

/cc @vesln

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

No branches or pull requests

2 participants