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

fix: replace openDetail with quoting q #1134

Merged
merged 1 commit into from
Jun 21, 2020
Merged

Conversation

erights
Copy link
Member

@erights erights commented May 24, 2020

This combines the declassification of openDetail with the quoting of the q 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 a toString stringification first. This has pros and cons. Looking for feedback.

@erights erights requested a review from kriskowal May 24, 2020 10:40
@erights erights force-pushed the openDetail-becomes-q branch from 16bf463 to a8ad873 Compare May 24, 2020 10:48
@erights
Copy link
Member Author

erights commented May 24, 2020

I just changed my mind and revised it to JSON.stringify without first doing toString. But question still open. Still looking for feedback.

@erights erights force-pushed the openDetail-becomes-q branch from a8ad873 to ffd671f Compare May 24, 2020 10:55
Copy link
Member

@kriskowal kriskowal left a 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.

const result = harden({
payload,
toString() {
return payload.toString();
return JSON.stringify(payload);
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed!

@erights erights force-pushed the openDetail-becomes-q branch 3 times, most recently from 661958a to 3b982f5 Compare May 28, 2020 19:54
@erights erights requested a review from kriskowal May 29, 2020 05:46
@erights
Copy link
Member Author

erights commented May 29, 2020

@kriskowal I did not intend to hit the "request rereview" button just now. I will do so on purpose soon, but not yet.

@erights erights self-assigned this May 29, 2020
@erights erights added assert package: assert tooling repo-wide infrastructure zoe-alpha-release labels May 29, 2020
@erights erights force-pushed the openDetail-becomes-q branch 3 times, most recently from 9f7aa03 to d1c1541 Compare May 30, 2020 03:55
@erights
Copy link
Member Author

erights commented May 30, 2020

@kriskowal , future possibilities to keep vaguely in mind while reviewing:

Moving this assert package into SES-shim, replacing the assert module there.

If we make SES-shim depend on Jessie for what SES and Jessie have in common, then we might want to move this assert package there instead. (attn @michaelfig )

I expect this this assert package will eventually be coupled to our error-stack shim and a console emulator, so the substitution values can be recovered from the other "side" of the console for debugging purposes. Also to keep in mind when we examine other logging possibilities. (attn @warner )

This assert package indicates failure by throwing an error. This is appropriate for failures

  • that our caller should be able to react to and recover from.

We need a way to indicate that, instead,

  • this turn-or-crank should be aborted and the vat should regress back to the last consistent state. (attn @dtribble )
  • this vat is confused and should be permanently terminated.

The error that's thrown is always a direct instance of Error. We should have some way, perhaps in the details part of the API, to indicate which specific error constructor to use.

Copy link
Member

@kriskowal kriskowal left a 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”.

@erights erights force-pushed the openDetail-becomes-q branch 2 times, most recently from 4abe69e to 1ed41c8 Compare June 21, 2020 20:01
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.
@erights erights force-pushed the openDetail-becomes-q branch from 1ed41c8 to bf2502c Compare June 21, 2020 20:16
@erights erights merged commit 67808a4 into master Jun 21, 2020
@erights erights deleted the openDetail-becomes-q branch June 21, 2020 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert package: assert tooling repo-wide infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants