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

PIR Generators #4559

Closed
wants to merge 135 commits into from
Closed

Conversation

MaximilianAlgehed
Copy link
Contributor

@MaximilianAlgehed MaximilianAlgehed commented Apr 27, 2022

This PR introduces QuickCheck generators for PIR.

Pre-submit checklist:

  • Branch
    • Tests are provided (if possible)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
    • Relevant tickets are mentioned in commit messages
    • Formatting, materialized Nix files, PNG optimization, etc. are updated
  • PR
    • (For external contributions) Corresponding issue exists and is linked in the description
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested

@michaelpj
Copy link
Contributor

What's the plan here? Are you planning to tidy this up more, or do you want a review right now?

@MaximilianAlgehed
Copy link
Contributor Author

MaximilianAlgehed commented Apr 27, 2022 via email

@effectfully
Copy link
Contributor

I'm almost done reviewing and addressing comments (I only have one more builtins-related thing in mind) on the types part of this PR. I've created a separate PR for it: #4880.

@MaximilianAlgehed take a look at that PR if you feel like it.

@MaximilianAlgehed
Copy link
Contributor Author

MaximilianAlgehed commented Oct 11, 2022 via email

@effectfully
Copy link
Contributor

effectfully commented Oct 11, 2022

@MaximilianAlgehed I think you're looking at some old comment. You've already addressed negativeVars and you've addressed all of my documentation requests as far as I remember (thank you for all of that!). I concluded that we have enough here for the types part to get merged, so I created a PR with only the types part with the intention to merge it, however Michael left another bunch of dozens of comments and I'm addressing those there. No activity is happening in this PR, so please don't spend your time fulfilling any requests here as we moved all activity to #4880. I'll ping you explicitly there on every comment that requires your attention (it's a fairly small percentage of comments I think, I'm mostly addressing things by myself). Hope that makes sense.

@effectfully
Copy link
Contributor

@MaximilianAlgehed the types part of this PR is merged! I'm now looking into the terms part.

@MaximilianAlgehed
Copy link
Contributor Author

Good news!

@effectfully
Copy link
Contributor

The term generators are merged too! There's still a lot to do, but that's for follow-ups.

Closing this PR. Thanks a lot @MaximilianAlgehed, for the code, for all the help and for patience!

@effectfully effectfully deleted the PR-PIR-generators branch January 10, 2023 14:23
@MaximilianAlgehed
Copy link
Contributor Author

Great! Let me know if you need any more feedback or information!

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.

4 participants