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

Replace push(result, actual, expected, message, negative) with object signature #827

Closed
jzaefferer opened this issue Jun 18, 2015 · 6 comments
Closed

Replace push(result, actual, expected, message, negative) with object signature #827

jzaefferer opened this issue Jun 18, 2015 · 6 comments
Labels
help welcome

Comments

@jzaefferer
Copy link
Member

As discussed in #822, the now even longer signature for .push() is bad. Especially with ES6 an object signature would work very well, .push({ result, actual, expected, message, negative }).

As I also mentioned in that PR: If we deprecate the current signature anyway, we might as well come up with a better name, since push is very generic. Any ideas for a better name?
@jzaefferer jzaefferer mentioned this issue Jun 18, 2015
Closed
@platinumazure
Copy link
Contributor

Regarding a different name: How about something like pushResult or addResult? The information being added is a test assertion result, but pushTestAssertionResult seems a bit verbose.

Certainly open to other suggestions, of course.

@leobalter
Copy link
Member

pushResult is interesting, as resolve would be a good inspiration from Promises.

@platinumazure
Copy link
Contributor

Seems resolve might be confusing with promises and assert.async (just conceptually).

@gabrielschulhof
Copy link

How about just result()?

@JamesMGreene
Copy link
Member

Personally, I would say assert... but that will likely cause some confusion with extensions needing to callthis(...) instead of this.pushResult(...).

@jzaefferer
Copy link
Member Author

Let's go with .pushResult({ result, actual, expected, message, negative }). The existing push() method can be updated to map to the newly added method.

mixed added a commit to mixed/qunit that referenced this issue Jan 21, 2016
@mixed
Assert: Change push to pushResult
b93b9ea 
Ref qunitjsgh-827
@mixed mixed mentioned this issue Jan 21, 2016
Closed
flore77 pushed a commit to flore77/qunit that referenced this issue Aug 10, 2016
@mixed @leobalter
Assert: Implement Assert#pushResult
cf8a1c1 
Fixes qunitjsgh-827
Closes qunitjsgh-920

The new pushResult method replaces Assert#push and works in the same way, but
with single parameter to fetch all the data .push received as individual
parameters.

The current assertions already use the new method, while .push is still present,
and it's now a deprecated method, expected to be removed on the next major
version, but updated to map to the newly added method.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help welcome
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Assert: Change push to pushResult mixed/qunit
5 participants
@jzaefferer @platinumazure @leobalter @JamesMGreene @gabrielschulhof