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: use util.inspect() to create error messages #668

Closed
wants to merge 1 commit into from
Closed

assert: use util.inspect() to create error messages #668

wants to merge 1 commit into from

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jan 30, 2015

Currently, JSON.stringify() is used to create error messages on failed assertions. This causes an error when stringifying objects with circular references. This commit switches out JSON.stringify() for util.inspect(), which can handle circular references.

This is a port of nodejs/node-v0.x-archive#8734. The main question here is about semver labeling.

cc: @piscisaureus

@piscisaureus
Copy link
Contributor

removing the depth completely and letting inspect() use its default of 2?

That would be my suggestion.

I'm +1 on this patch. I think the biggest question is whether this is semver-major or minor. I think this is a minimal behavioral change for which a minor version bump is appropriate.

Currently, JSON.stringify() is used to create error messages
on failed assertions. This causes an error when stringifying
objects with circular references. This commit switches out
JSON.stringify() for util.inspect(), which can handle
circular references.
@chrisdickinson
Copy link
Contributor

I'm in favor of semver patch here: it's not new "behavior" – in the form of a new API method or parameter, and doesn't change the contract – trying to JSON.parse will still explode, just more often now :)

I'm also +1 on this patch.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 30, 2015

I have updated the PR.

Since the window is currently open for a minor version, I'll land this tomorrow if no one protests.

@Fishrock123 Fishrock123 added this to the v1.1.0 milestone Jan 30, 2015
cjihrig added a commit that referenced this pull request Jan 31, 2015
Currently, JSON.stringify() is used to create error messages
on failed assertions. This causes an error when stringifying
objects with circular references. This commit switches out
JSON.stringify() for util.inspect(), which can handle
circular references.

PR-URL: #668
Reviewed-By: Julien Gilli <julien.gilli@joyent.com>
Reviewed-By: Bert Belder <bertbelder@gmail.com>
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 31, 2015

Landed in 40e29dc

@cjihrig cjihrig closed this Jan 31, 2015
@cjihrig cjihrig deleted the assert-inspect branch January 31, 2015 14:40
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.

4 participants