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

Symbol.toStringTag dominates default q stringify #604

Merged
merged 2 commits into from
Mar 12, 2021
Merged

Conversation

erights
Copy link
Contributor

@erights erights commented Mar 8, 2021

Previously, if an object had a property named by the Symbol.toStringTag symbol and it was empty, then the human readable "quoted" printing by q would be "[<value of that property>]".

This PR removes the "and it was empty" part of that condition. This PR should wait until Agoric/agoric-sdk#2018 is closed, because until then data objects, whether empty or not, will pass this test, preventing q from providing a pleasant display of their properties.

Until then, however, we have a painful difference in how q renders a remotable vs a remote presence of that remotable. Currently, for our objects-as-closures style of remotable, q will show the method names. This difference means that t.throws tests will see different quoted text in error messages depending on where the error was thrown. Attn @Chris-Hibbert

@erights erights self-assigned this Mar 8, 2021
Copy link
Contributor

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

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

ack.

One comment needs to be updated.

Please ask me to review again when it's time to submit.

packages/ses/src/error/stringify-utils.js Show resolved Hide resolved
@erights erights changed the title WIP Symbol.toStringTag dominates default q stringify Symbol.toStringTag dominates default q stringify Mar 12, 2021
@erights erights marked this pull request as ready for review March 12, 2021 20:59
@erights
Copy link
Contributor Author

erights commented Mar 12, 2021

Given the progress on #2018, this is now ready for review.

(In fact, this did not need to wait on that. It did not have the conflict with the old Data presentation I thought it did. Not that this matters anymore ;) )

@erights erights merged commit b40bfa5 into master Mar 12, 2021
@erights erights deleted the markm-simpler-q branch March 12, 2021 22:45
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.

2 participants