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

Declare many of our remotables #2178

Merged
merged 10 commits into from
Jan 19, 2021
Merged

Declare many of our remotables #2178

merged 10 commits into from
Jan 19, 2021

Conversation

erights
Copy link
Member

@erights erights commented Jan 11, 2021

To help track down the mysterious missing iface strings.

To make progress on #804

To make progress on #2018

@erights erights self-assigned this Jan 11, 2021
@erights erights requested a review from michaelfig January 11, 2021 07:52
@erights erights force-pushed the instrument-serialize branch from a9474e0 to bb743c0 Compare January 11, 2021 22:43
Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

Take my comments with a grain of salt, but eliminating typecasts is my main goal.

packages/zoe/src/contractFacet/contractFacet.js Outdated Show resolved Hide resolved
packages/zoe/src/makeHandle.js Outdated Show resolved Hide resolved
@erights erights force-pushed the instrument-serialize branch from 734b85c to 2d3d901 Compare January 15, 2021 06:40
@erights erights changed the title WIP Instrument serialize Declare many of our remotables Jan 15, 2021
@erights erights marked this pull request as ready for review January 15, 2021 07:33
@erights
Copy link
Member Author

erights commented Jan 15, 2021

Hi @michaelfig this is now ready for review. Thanks.

@erights
Copy link
Member Author

erights commented Jan 16, 2021

This is now blocked on #2207 which explains the three failures reported by CI.

@erights erights requested a review from michaelfig January 17, 2021 05:29
@erights
Copy link
Member Author

erights commented Jan 17, 2021

Hi @michaelfig I made a lot of changes since you approved. Could you review again? Thanks.

@erights erights force-pushed the instrument-serialize branch from 8ad59d5 to fc8a9a1 Compare January 17, 2021 05:33
@erights erights requested a review from FUDCo January 18, 2021 19:07
Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

A lot of these labels seem not to be strictly necessary, as they are not empty objects. Is there a reason to add them all?

Otherwise, LGTM.

Copy link
Contributor

@FUDCo FUDCo left a comment

Choose a reason for hiding this comment

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

This is not what I was expecting. I had, perhaps mistakenly, framed the problem as "empty objects are treated as presences rather than data, which makes them dangerous when they are actually data" and I assumed the fix was to change serialization to always treat empty objects as data unless explicitly declared otherwise. Making this change requires us to first label all the empty objects that are currently being used as presences (mainly as identity markers, if I understand correctly), which is what I thought you were doing. This is why I was surprised to hear that there were many cases of things that needed to be so declared in SwingSet, since it doesn't (as best I can recall) make much, if any, use of the empty-object-passed-for-its-identity pattern. But here I see this PR declaring lots of Remotables that are not empty objects, which, while it might eventually be useful from a documentary perspective, seems unrelated to the underlying problem I thought we were trying to fix. Instead, it seems like we're just adding a lot of boilerplate noise with little payoff.

Also, I think the word "Alleged" as used here has jumped the shark. If all Remotable types are alleged types (are they?), then labelling each one as such is empty ceremony -- it takes up space but the reader's brain learns to ignore it since it conveys no new information.

@erights erights force-pushed the instrument-serialize branch from b8441cd to 3c22937 Compare January 19, 2021 02:45
@erights erights requested a review from FUDCo January 19, 2021 02:46
@erights
Copy link
Member Author

erights commented Jan 19, 2021

Ok @FUDCo , responded as we discussed. Ready for your rereview. Thanks.

Copy link
Contributor

@FUDCo FUDCo left a comment

Choose a reason for hiding this comment

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

That's nicer.

@erights erights merged commit 648b297 into master Jan 19, 2021
@erights erights deleted the instrument-serialize branch January 19, 2021 03:09
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.

3 participants