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 assert on sandbox instances #1075

Closed
ljharb opened this issue Jun 28, 2016 · 8 comments
Closed

Expose assert on sandbox instances #1075

ljharb opened this issue Jun 28, 2016 · 8 comments

Comments

@ljharb
Copy link
Contributor

ljharb commented Jun 28, 2016

Per https://github.com/wealthfront/sinon-sandbox/issues/7:

In our codebase, we use import sinon from 'sinon-sandbox', and have a linter rule that forbids importing sinon directly. However, this means we can't use sinon.assert.

Would you be willing to accept a PR that attaches sinon.assert to sandbox instances?

@mantoni
Copy link
Member

mantoni commented Jun 28, 2016

Maybe you can use sinon.assert.expose to attach it yourself? See http://sinonjs.org/docs/#assertions

@ljharb
Copy link
Contributor Author

ljharb commented Jun 28, 2016

@mantoni according to http://sinonjs.org/docs/#assert-expose, that means that the actual sinon.assert API won't be there - ie, sinon.assert.called will become assertCalled - the whole idea is that sinonSandbox.assert has identical methods as sinon.assert :-/

@mantoni
Copy link
Member

mantoni commented Jun 28, 2016

Well, then you can add the following here:

sandbox.assert = sinon.assert;

Or is there more to it?

@ljharb
Copy link
Contributor Author

ljharb commented Jun 28, 2016

Indeed! Per https://github.com/wealthfront/sinon-sandbox/issues/7#issuecomment-228912571 however, I think it would be better for the upstream to do it - since the sandbox already exposes match, the precedent is there.

Is there a reason that this semver-minor change wouldn't be a good thing?

@mantoni
Copy link
Member

mantoni commented Jun 28, 2016

Fine with me, was just curious. We're going to review the API surface for the 3.0 release and I wouldn't want to introduce anything that doesn't fit with that. Maybe @mroderick or @fatso83 have any feelings?

@fatso83
Copy link
Contributor

fatso83 commented Jun 28, 2016

I just reviewed #1000 (offtopic: we should try and get this ball rolling again) and I could not really see much preventing us from doing this. It is a very minor change AFAIK, and seemingly only aligns the sandbox api with that of the sinon object.

So I am pro.

@ljharb
Copy link
Contributor Author

ljharb commented Jun 29, 2016

Submitted #1076

ljharb added a commit to ljharb/sinon that referenced this issue Jun 29, 2016
fatso83 pushed a commit that referenced this issue Jun 29, 2016
@ljharb
Copy link
Contributor Author

ljharb commented Jun 29, 2016

Yay, thanks everyone! Looking forward to the release.

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

3 participants