Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

cjihrig
Copy link

@cjihrig cjihrig commented Nov 17, 2014

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.

Closes #8696

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.
@piscisaureus
Copy link

This is a good idea. +1

@misterdjules
Copy link

LGTM, thanks @cjihrig!

misterdjules pushed a commit to misterdjules/node that referenced this pull request Jan 29, 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: nodejs#8734
PR-URL: nodejs#8734
Reviewed-By: Julien Gilli <julien.gilli@joyent.com>
@misterdjules
Copy link

All tests pass (unsurprisingly), landed in bcff90e.

@piscisaureus
Copy link

@cjihrig Would be good to land this in io.js as well.

@cjihrig
Copy link
Author

cjihrig commented Jan 30, 2015

@piscisaureus this could be considered a breaking change. Thoughts on that?

@TooTallNate
Copy link

I would agree

@piscisaureus
Copy link

@cjihrig Semver-minor? The merge window for 1.1 is open.

@cjihrig
Copy link
Author

cjihrig commented Jan 30, 2015

@piscisaureus I would say semver-major because perfectly valid programs could stop working if they were doing any string matching on the output. I'll open a PR over there and see what others say.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants