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

[pickles] Document Hlist #10232

Merged
merged 2 commits into from
Feb 16, 2022
Merged

[pickles] Document Hlist #10232

merged 2 commits into from
Feb 16, 2022

Conversation

Firobe
Copy link
Contributor

@Firobe Firobe commented Feb 12, 2022

Add considerable documentation to Hlist in the pickles lib, in a new MLI file.

Also renames Fst and Snd to Arg1 and Arg2, and removes Hlist_1, which is redundant with Hlist0.H1_1.

@Firobe Firobe requested a review from a team as a code owner February 12, 2022 00:23
@Firobe Firobe changed the title Document hlist [pickles] Document Hlist Feb 12, 2022
Copy link
Contributor

@mimoo mimoo left a comment

Choose a reason for hiding this comment

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

At a glance this looks great o_O!! Two small questions:

  • did you manage to build HTML documentation out of these files? I couldn't when I tried
  • unrelated to this PR, but do you think this should live outside of pickles, as its own library or as part of a utility library?

@Firobe
Copy link
Contributor Author

Firobe commented Feb 12, 2022

At a glance this looks great o_O!! Two small questions:

  • did you manage to build HTML documentation out of these files? I couldn't when I tried

Yup! I had to build the whole documentation for the repo by running dune build @doc at the root. There are a number of errors but they can be ignored. I temporarily hosted a copy of the generated documentation here if you want to browse it.

  • unrelated to this PR, but do you think this should live outside of pickles, as its own library or as part of a utility library?

I'd love to have a generic HList library living completely outside of Mina. However here Hlist is tightly tied to the other types in pickles_types, so I don't think it would be possible or desirable to completely isolate Hlists from this library. Furthermore there really are not a lot of situations where you would want to use these lists in general, so they won't be reused much anyway.

@Firobe Firobe requested a review from a team as a code owner February 12, 2022 11:46
@Firobe Firobe force-pushed the document-hlist branch 2 times, most recently from 349f50f to 5bfc4bc Compare February 12, 2022 11:49
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.

This looks great to me! I've sprinkled a few nits, but this is already a huge improvement.

1 caveat: I suspect you didn't intend to update the snarky submodule 😁

@Firobe Firobe force-pushed the document-hlist branch 2 times, most recently from d679232 to ebebe99 Compare February 15, 2022 17:51
@Firobe Firobe requested a review from a team as a code owner February 15, 2022 17:51
@Firobe Firobe force-pushed the document-hlist branch 2 times, most recently from 8fd9618 to dd9f323 Compare February 15, 2022 18:30
@Firobe
Copy link
Contributor Author

Firobe commented Feb 15, 2022

1 caveat: I suspect you didn't intend to update the snarky submodule 😁

Removed that nasty change from the commit 😅 Good work for noticing that

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.

Excellent!

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.

Submodule updates have slipped back in.

(Also, if you force push rather than adding new commits, I can't easily track what's changed between one review and the next, which is a pain.)

@Firobe
Copy link
Contributor Author

Firobe commented Feb 15, 2022

Right, sorry. Force-pushed a last time to remove the submodule updates again. I like to keep a clean history but I guess I should rebase at the very end before merging instead of doing it continuously.

@Firobe Firobe added the ci-build-me Add this label to trigger a circle+buildkite build for this branch label Feb 15, 2022
@mrmr1993
Copy link
Member

I like to keep a clean history but I guess I should rebase at the very end before merging instead of doing it continuously.

We've been running with a policy of 'merge what was approved' and 'don't change branches out from under reviewers'. Our git history is a mess, but reviews and merges are way easier than they were before.

@Firobe Firobe added the ready-to-merge Adding this label will trigger mergify and trigger CI to run and merge the PR label Feb 16, 2022
@mrmr1993 mrmr1993 merged commit 62f669d into develop Feb 16, 2022
@mrmr1993 mrmr1993 deleted the document-hlist branch February 16, 2022 19:16
@robinbb robinbb added the Tweag label Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-build-me Add this label to trigger a circle+buildkite build for this branch ready-to-merge Adding this label will trigger mergify and trigger CI to run and merge the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants