-
Notifications
You must be signed in to change notification settings - Fork 212
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
fix: replace openDetail with quoting q #1134
Conversation
16bf463
to
a8ad873
Compare
I just changed my mind and revised it to |
a8ad873
to
ffd671f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of renaming openDetail
to q
and for it to be the more general form of what I’ve been doing with q
in SES strictly for string details.
To account for cycles in the JSON representation of a detail, we might be able to use a JSON reviver and replace cycles with ibid references. The Node.js utils
built-in module exports the function it uses for representing objects with console.log, but I hesitate to use a built-in module for anything that might also be used on the web.
packages/assert/src/assert.js
Outdated
const result = harden({ | ||
payload, | ||
toString() { | ||
return payload.toString(); | ||
return JSON.stringify(payload); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this will throw RangeError for payloads that contain cyclic graphs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You meant a JSON replacer, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed!
661958a
to
3b982f5
Compare
@kriskowal I did not intend to hit the "request rereview" button just now. I will do so on purpose soon, but not yet. |
9f7aa03
to
d1c1541
Compare
@kriskowal , future possibilities to keep vaguely in mind while reviewing: Moving this If we make SES-shim depend on Jessie for what SES and Jessie have in common, then we might want to move this I expect this this This assert package indicates failure by throwing an error. This is appropriate for failures
We need a way to indicate that, instead,
The error that's thrown is always a direct instance of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ibid replacer seems fine.
nit: I do love Latinisms like /ibid/, but for clarity to the folks who would have to look it up, I favor the name “seen”. I’ve worked with majority-ESL teams that have even convinced me that “indexes” is better than “indices”.
4abe69e
to
1ed41c8
Compare
The new `q` both declassifies, as the old `openDetail` did, as well as doing a modified `JSON.stringify`, to quote the declassified contents as placed into the thrown error's message. The unquoted data itself is still sent to the console for better debugging if the console supports that. `q` differs from `JSON.stringify` in being cycle tolerant. It will print subtrees it has already seen as "<**seen**>", collapsing both dags and cycles.
1ed41c8
to
bf2502c
Compare
This combines the declassification of
openDetail
with the quoting of theq
from @kriskowal's assertion style. It makes sense that if you're putting the quoted form of the stringified payload into the message, then you must declassify it anyway, so it is unsurprising.Open question: @kriskowal just did a
JSON.stringify
directly, without doing atoString
stringification first. This has pros and cons. Looking for feedback.