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

Alias spy.reset to spy.resetHistory #1630

Merged
merged 4 commits into from Dec 11, 2017
Merged

Alias spy.reset to spy.resetHistory #1630

merged 4 commits into from Dec 11, 2017

Conversation

GCHQDeveloper500
Copy link
Contributor

@GCHQDeveloper500 GCHQDeveloper500 commented Dec 8, 2017

Purpose (TL;DR) - mandatory

As per #1446 - deprecates and aliases spy.reset in favour of spy.resetHistory

Background (Problem in detail) - optional

See ticket.

Solution - optional

  • Change existing spy.reset test to instead use spy.resetHistory
  • Introduce spy.reset test to make sure alias works
  • Implement changes (just a function name change and an alias)
  • Updated docs to reflect changes

How to verify - mandatory

  1. Check out this branch
  2. npm install
  3. npm test

Checklist for author

  • npm run lint passes
  • References to standard library functions are cached.

Copy link
Contributor

@fatso83 fatso83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. Some minor changed required.


Resets the state of a spy.


#### `spy.reset()`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this. If it's deprecated we don't need to spread knowledge of its existence. Only old code-bases should use this.

lib/sinon/spy.js Outdated
@@ -413,6 +413,8 @@ function delegateToCalls(method, matchAny, actual, notCalled) {
};
}

spyApi.reset = spyApi.resetHistory;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be be wrapped in our deprecation helper. See lib/sinon/util/core/deprecated.js

@GCHQDeveloper500
Copy link
Contributor Author

Thanks for the feedback @fatso83 I'll take a look after the weekend.

@GCHQDeveloper500
Copy link
Contributor Author

Okay @fatso83 this is ready for re-review - do you think we need a test for the deprecated reset method? Doesn't appear to have been something that people have done in the past.

@fatso83 fatso83 merged commit bde5b51 into sinonjs:master Dec 11, 2017
@fatso83
Copy link
Contributor

fatso83 commented Dec 11, 2017

This was fine, no worries. Thanks a lot!

@GCHQDeveloper500 GCHQDeveloper500 deleted the feature/alias-resethistory-to-history branch December 11, 2017 12:55
@mroderick
Copy link
Member

This has been released with sinon@4.1.4

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.

None yet

3 participants