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

Split marshal into passStyleOf and marshal #2803

Merged
merged 1 commit into from
Apr 8, 2021
Merged

Conversation

erights
Copy link
Member

@erights erights commented Apr 5, 2021

Almost purely internal cleanup.

I moved sameValueZero to same-structure package, which is the only place it is used. I don't even export it from there. I may reverse this before merging.

@erights erights self-assigned this Apr 5, 2021
@erights erights requested a review from michaelfig April 5, 2021 02:24
@erights
Copy link
Member Author

erights commented Apr 5, 2021

@michaelfig when you review this, could you also let us know here what we should do to make this dapp-card-store CI error go away? Thanks.

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.

LGTM!

I did not go over the code copying with a fine-toothed comb. If you changed any semantics in it, I'd like to know what that was so I can check it more carefully.

@erights
Copy link
Member Author

erights commented Apr 8, 2021

I did not go over the code copying with a fine-toothed comb. If you changed any semantics in it, I'd like to know what that was so I can check it more carefully.

One. As the PR comment says

I moved sameValueZero to same-structure package, which is the only place it is used. I don't even export it from there. I may reverse this before merging.

I looked again and confirmed that the only semantic change is moving sameValueZero from the marshal to the sameStructure package, where it is not exported. This is technically a breaking change, but doesn't seem to break anything because it really seems nothing is using it. But since it is technically a breaking change, I'd like your opinion on this specifically before merging. Thanks.

@erights erights requested a review from michaelfig April 8, 2021 01:14
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.

Yes, the sameValueZero change is fine!

@erights erights enabled auto-merge (squash) April 8, 2021 06:43
@erights erights merged commit 2e19e78 into master Apr 8, 2021
@erights erights deleted the split-marshall branch April 8, 2021 07:00
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