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

assert: change callTracker.calls and callTracker.callsWith to use an object param instead of positional params #43161

Closed
ErickWendel opened this issue May 20, 2022 · 6 comments
Labels
assert Issues and PRs related to the assert subsystem. feature request Issues that request new features to be added to Node.js. stale

Comments

@ErickWendel
Copy link
Member

ErickWendel commented May 20, 2022

What is the problem this feature will solve?

I've been implementing some features on the assert module see #43133 and I figured that it's a bit confusing for users to use the signature:

callTracker.calls()
callTracker.calls(1)
callTracker.calls(fn)
callTracker.calls(fn, 1)

callTracker.callsWith(['arg1'])
callTracker.callsWith(fn, ['arg1'])
callTracker.callsWith(fn, ['arg1'], 1)

When looking at the intelliSense, it's also confusing because the calls signature is calls(fn, exact = 1) however nether fn or exact are required fields so we have to check every positional argument see

// When calls([arg1, arg2], ?1)
if (ArrayIsArray(fn)) {
exact = typeof withArgs === 'number' ? withArgs : exact;
withArgs = fn;
fn = noop;
}
// When calls(1)
if (typeof fn === 'number') {
exact = fn;
fn = noop;
}
// When calls()
if (fn === undefined) {
fn = noop;
}
// Else calls(fn, 1, [])
validateUint32(exact, 'exact', true);

What is the feature you are proposing to solve the problem?

I'd like to propose that we use object destructuring for those function signatures as it could improve the editor's intelliSense and readability:

callTracker.calls({
  exact: 1,
  fn: myFn
})

callTracker.callsWith({
  withArgs: ['arg1']
})

What alternatives have you considered?

No response

@ErickWendel ErickWendel added the feature request Issues that request new features to be added to Node.js. label May 20, 2022
@VoltrexKeyva VoltrexKeyva added the assert Issues and PRs related to the assert subsystem. label May 20, 2022
@Trott
Copy link
Member

Trott commented May 20, 2022

@nodejs/assert @nodejs/testing

@Trott
Copy link
Member

Trott commented May 20, 2022

I'm not super-familiar with this API, but the suggestion for calls() to be calls(options) makes sense to me. For callsWith(), I wonder if we want callsWith(args, options).

@ErickWendel
Copy link
Member Author

I'm not super-familiar with this API, but the suggestion for calls() to be calls(options) makes sense to me. For callsWith(), I wonder if we want callsWith(args, options).

I liked it! both calls(options) and callsWith(args, options) 🤩

Should I implement it?

@ErickWendel
Copy link
Member Author

Should I? 😬

@jasnell
Copy link
Member

jasnell commented Jun 6, 2022

I'd say go ahead

ErickWendel added a commit to ErickWendel/node that referenced this issue Jun 7, 2022
…tional params

it changes the callTracker API, its docs and tests that uses this module. This is a breaking change as the calls function was changed

Refs: nodejs#43161
ErickWendel added a commit to ErickWendel/node that referenced this issue Jun 7, 2022
it changes the callTracker API, its docs and tests that use this module. This is a breaking change as the calls function was changed

Refs: nodejs#43161
ErickWendel added a commit to ErickWendel/node that referenced this issue Jun 7, 2022
it changes the callTracker API, its docs and tests that use this module

Refs: nodejs#43161
ErickWendel added a commit to ErickWendel/node that referenced this issue Jun 7, 2022
it changes the callTracker API, its docs and tests that use this module

Refs: nodejs#43161
ErickWendel added a commit to ErickWendel/node that referenced this issue Jun 7, 2022
it changes the callTracker API, its docs and tests that use this module

Refs: nodejs#43161
ErickWendel added a commit to ErickWendel/node that referenced this issue Jun 7, 2022
it changes the callTracker API, its docs and tests that use this module

Refs: nodejs#43161
ErickWendel added a commit to ErickWendel/node that referenced this issue Jun 7, 2022
it changes the callTracker API, its docs and tests that use this module

Refs: nodejs#43161
@targos targos moved this to Pending Triage in Node.js feature requests Oct 22, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 4, 2022

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Dec 4, 2022
@targos targos moved this from Pending Triage to Stale in Node.js feature requests Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. feature request Issues that request new features to be added to Node.js. stale
Projects
None yet
Development

No branches or pull requests

4 participants