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 createStubInstance header in stubs.md #2576

Merged
merged 1 commit into from
Sep 10, 2024
Merged

Conversation

DanKaplanSES
Copy link
Contributor

Purpose (TL;DR) - mandatory

I kept scrolling past the parts of this page that explain createStubInstance so I created a section for it. I think the reason it didn't already exist is because it's considered a part of the utilities API, not the stub API. Even if that's the reasoning, I hope this PR is still a net positive, because personally, it makes it easier to find what I'm looking for.

How to verify - mandatory

  1. Check out this branch
  2. npm install

The documentation contribute in guide led me to believe that this doesn't apply to documentation changes. Let me know if I misinterpreted it, though.

Checklist for author

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

I kept scrolling past the parts of this page that explain `createStubInstance` so I created a section for it. I think the reason it didn't already exist is because it's considered a part of the utilities API, not the stub API. Even if that's the reasoning, I hope this PR is still a net positive, because personally, it makes it easier to find what I'm looking for.
Comment on lines +93 to 95
#### `var stubInstance = sinon.createStubInstance(MyConstructor, overrides);`

If you want to create a stub object of `MyConstructor`, but don't want the constructor to be invoked, use this utility function.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe expose the fact that this method really is part of the sandbox? As in sandbox.createStubInstance.... Otherwise people might mix both sinon and sandbox invocations in the same file, I think.

Whatcha think?

Copy link
Contributor Author

@DanKaplanSES DanKaplanSES Jan 10, 2024

Choose a reason for hiding this comment

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

@fatso83 Yeah I wasn't even aware of that. Here's my knee-jerk thought: on this line, I think changing sinon to sandbox might confuse the reader more than it informs them: some readers may not know what sandbox is (yet) and assume they can't call this method on sinon. Maybe that's a good thing, but the sandbox docs say:

Unless you have a very advanced setup or need a special configuration, you probably only need to use that one in your tests for easy cleanup.

This could be far-fetched, but they might extrapolate that to mean createStubInstance is only for "very advanced setup or need a special configuration," too. For all I know, that's the intended interpretation, but I'm assuming it's not. I'm assuming it's used more often than non-sinon sandboxes.

That said, I do think this is useful information. Maybe it could be added as a detail below the header or in the link added by #2577 ?

I don't feel that strongly about anything I've said above: if you disagree with my take, I'm happy to make your suggested change.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about keeping the sinon.create... bits, but link to the sandbox.md docs for more elaborate examples?

Copy link

codecov bot commented Jan 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.02%. Comparing base (adcf936) to head (123df37).
Report is 30 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2576   +/-   ##
=======================================
  Coverage   96.02%   96.02%           
=======================================
  Files          40       40           
  Lines        1912     1912           
=======================================
  Hits         1836     1836           
  Misses         76       76           
Flag Coverage Δ
unit 96.02% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fatso83
Copy link
Contributor

fatso83 commented Feb 15, 2024

I think this was mostly done, apart from a few tiny changes. Any chance you will get to complete this? Think it's less than 10 minutes of work.

@fatso83
Copy link
Contributor

fatso83 commented Apr 25, 2024

@DanKaplanSES ? should I move it over the finish line?

@DanKaplanSES
Copy link
Contributor Author

@DanKaplanSES ? should I move it over the finish line?

Apologies, yeah I think that would be best; I don't fully understand what change you're suggesting.

@fatso83 fatso83 merged commit 5a7924a into sinonjs:main Sep 10, 2024
9 checks passed
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.

2 participants