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.

Sign up for GitHub

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

Jump to bottom

add documentation about sinon.restoreObject() #2106

Merged
merged 1 commit into from
Oct 2, 2019
Merged

add documentation about sinon.restoreObject() #2106

merged 1 commit into from
Oct 2, 2019

Conversation

franck-romano
Copy link

@franck-romano franck-romano commented Oct 1, 2019 •
edited

Add documentation about sinon.restoreObject() related to #2089

How to verify

  1. Check out this branch
  2. npm install
  3. Start documentation (https://github.com/sinonjs/sinon/blob/master/docs/CONTRIBUTING.md)
  4. Then go to the utils section and see sinon.restoreObject(object);

@fearphage
Copy link
Member

@franck-romano Thank you so much. We really appreciate your contribution.

A few things to add:

  1. Can you please catch your branch up to master? That will fix the merge conflict and the tests failing.
    image
  2. There are some additional changes coming to restoreObject that are in code review right now (Stricter object iteration #2092). Could you add a note that this will throw if it doesn't restore anything? So sinon.restoreObject({}) will throw an error since it will fail to restore any spies, stubs, etc.

@franck-romano franck-romano force-pushed the add-documentation-about-restore-object branch from 18ab840 to 30cbb6e Compare October 1, 2019 17:03
@franck-romano franck-romano force-pushed the add-documentation-about-restore-object branch from 3be2608 to 82f0a9b Compare October 1, 2019 17:26
@franck-romano
Copy link
Author

franck-romano commented Oct 1, 2019 •
edited

@fearphage Yes sorry, I didn't know why master was so outdated ...
It's done, tell me if you see something else and if it fits your needs ! :)

@franck-romano franck-romano reopened this Oct 1, 2019
fearphage
fearphage requested changes Oct 1, 2019
View reviewed changes
Copy link
Member

@fearphage fearphage left a comment

Choose a reason for hiding this comment

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

It looks good. Thanks for adding the blurb about throwing errors. I have a few small suggestions.

docs/release-source/release/utils.md Outdated

### `sinon.restoreObject(object);`

Restores all methods of an object and returns the restored object
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Restores all methods of an object and returns the restored object
Restores all methods of an object and returns the restored object.

Copy link
Author

Choose a reason for hiding this comment

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

Done ✔️

docs/release-source/release/utils.md Outdated
sinon.restoreObject(obj);
```

Throws an error if the object is empty since no stubs, spies or mock are restorable
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Throws an error if the object is empty since no stubs, spies or mock are restorable
Throws an error if the object contains no restorable methods (spies, stubs, etc).

Copy link
Author

Choose a reason for hiding this comment

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

Done ✔️

@franck-romano franck-romano force-pushed the add-documentation-about-restore-object branch from 710167c to af45f4e Compare October 1, 2019 18:39
fearphage
fearphage approved these changes Oct 1, 2019
View reviewed changes
Copy link
Member

@fearphage fearphage left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks again!

I'll give a little time in case anyone else wants to chime in.

@franck-romano
Copy link
Author

Nice, it was a pleasure.

Keep it up the good work, sinon is a very nice library !

mantoni
mantoni approved these changes Oct 2, 2019
View reviewed changes
Copy link
Member

@mantoni mantoni left a comment

Choose a reason for hiding this comment

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

Nice, thank you!

@mantoni mantoni merged commit cd58c87 into sinonjs:master Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fearphage fearphage fearphage approved these changes

@mantoni mantoni mantoni approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@franck-romano @fearphage @mantoni