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

Vinegar RFC #42

Merged
merged 15 commits into from
Feb 14, 2024
Merged

Vinegar RFC #42

merged 15 commits into from
Feb 14, 2024

Conversation

volhovm
Copy link
Member

@volhovm volhovm commented Jan 16, 2024

This introduces the RFC describing vinegar, the compatibility layer for pickles.

PRs merging documentation for pickles created when sketching this RFC:

@volhovm volhovm marked this pull request as ready for review January 22, 2024 21:10
Copy link
Member

@mrmr1993 mrmr1993 left a comment

Choose a reason for hiding this comment

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

Broadly looks good, apart from a few structural comments. There are a few issues though:

  • this doesn't follow the issue template, and essentially only addresses the Detailed design section thereof;
  • this doesn't have an RFC number (although I can quickly allocate one when this is ready to merge).

00xx-vinegar.md Outdated Show resolved Hide resolved
00xx-vinegar.md Outdated Show resolved Hide resolved
00xx-vinegar.md Outdated Show resolved Hide resolved
00xx-vinegar.md Outdated Show resolved Hide resolved
00xx-vinegar.md Outdated Show resolved Hide resolved
@volhovm
Copy link
Member Author

volhovm commented Feb 6, 2024

@mrmr1993 Thank you for the feedback.

  • Will address the comments and introduce the template -- didn't realise there was one at the point of writing.
  • Regarding the number -- yes, I thought we just assign a number at merge.

@@ -23,7 +23,7 @@ In general:

* Be specific. This document is meant to share intent to your colleagues. Share what you believe you will actually do.
* Be decisive. No maybes. Any uncertainty can be captured in the unresolved questions section at the end.
* Provide design contex so that we can align on and commit to a technical design.
Copy link
Member Author

Choose a reason for hiding this comment

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

For the reviewers: changes in 001-template.md are orthogonal, but very cosmetic, and it would be very hard to merge a separate PR because it needs a lot of approvals. So I want to add it here. It's a separate commit, so can be reverted individually if needed.

Copy link
Member

@mrmr1993 mrmr1993 left a comment

Choose a reason for hiding this comment

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

Detail LGTM. See comments for spelling nits, and an answer to the unresolved question.

Consider inlining the answer and removing the unresolved question before merging, and please choose the next sequential RFC number as you merge.

00xx-vinegar.md Outdated Show resolved Hide resolved
00xx-vinegar.md Outdated Show resolved Hide resolved
00xx-vinegar.md Outdated Show resolved Hide resolved
00xx-vinegar.md Outdated Show resolved Hide resolved
@volhovm
Copy link
Member Author

volhovm commented Feb 14, 2024

@mrmr1993 all addressed, assigned number 12 (next sequential after zkapps), merging.

@volhovm volhovm added this pull request to the merge queue Feb 14, 2024
Merged via the queue into main with commit 8550a60 Feb 14, 2024
@volhovm volhovm deleted the volhovm/vinegar-rfc branch February 14, 2024 12:26
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