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

[sinon] implement on{,First,Second,Third}Call([...]) #361

Merged
merged 3 commits into from
Aug 23, 2022
Merged

[sinon] implement on{,First,Second,Third}Call([...]) #361

merged 3 commits into from
Aug 23, 2022

Conversation

danbeam
Copy link
Contributor

@danbeam danbeam commented Aug 20, 2022

Reviewers (other than @skovhus): @catc @lencioni @jayrobin

@codecov
Copy link

codecov bot commented Aug 20, 2022

Codecov Report

Merging #361 (e1f57d6) into main (6128a21) will increase coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #361      +/-   ##
==========================================
+ Coverage   92.19%   92.30%   +0.11%     
==========================================
  Files          26       26              
  Lines        1896     1924      +28     
  Branches      398      404       +6     
==========================================
+ Hits         1748     1776      +28     
  Misses        102      102              
  Partials       46       46              
Impacted Files Coverage Δ
src/transformers/sinon.ts 89.92% <100.00%> (+1.17%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

`
stub.mockImplementation(() => {
if (stub.mock.calls.length === 2)
return [8, 9, 10];
Copy link
Owner

Choose a reason for hiding this comment

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

Personally I find one-line statements less readable and error prone.

It would be slightly nicer with an arrow function here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hey @skovhus, I'm not sure I understand what you're asking for here.

do you mean braces around the if?

      stub.mockImplementation(() => {
            if (stub.mock.calls.length === 2) {
                  return [8, 9, 10];
            }

you said "arrow function" so maybe you mean this?

      stub.mockImplementation(() => stub.mock.calls.length === 2 && [8, 9, 10]);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because the second route (stub.mockImplementation(() => stub.mock.calls.length === 2 && [8, 9, 10]);) would sometimes produce false for a return value (which differs from sinon)

const sinon = require('sinon');

const s = sinon.stub();

s.onFirstCall().returns(5);

console.log(s());  // 5
console.log(s());  // undefined

i went with braces around the single-line if statements. hopefully that's what you meant.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes I would prefer us to either use block statements or arrow functions instead of the one-line statements. Thanks for fixing this.

@skovhus skovhus merged commit b4ca35b into skovhus:main Aug 23, 2022
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.

3 participants