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

irmin.type: test the pp_ty operation #968

Merged
merged 4 commits into from
Mar 14, 2020
Merged

Conversation

craigfe
Copy link
Member

@craigfe craigfe commented Mar 10, 2020

Resolves #959. In the same vein as #965, test the behaviour of the Irmin.Type.pp_ty operation. Found and fixed a format string bug in doing so.

Shares a LCA with #965 for the ppx_irmin dependency.

Some thoughts about the current behaviour:

  • pretty-printed output for algebraic types isn't particularly helpful (c.f. irmin.type: pp_ty should show components of algebraic types #958);
  • the Prim node could be skipped during pretty-printing;
  • Custom (foo) is really Like (foo) from a user perspective;
  • we could consider a more OCaml-friendly ordering & capitalisation (e.g. int list rather than List (Prim Int)).

@craigfe
Copy link
Member Author

craigfe commented Mar 11, 2020

Rebased.

@craigfe craigfe force-pushed the test-pp-ty branch 2 times, most recently from 246191b to 5ab226a Compare March 13, 2020 09:25
@craigfe
Copy link
Member Author

craigfe commented Mar 13, 2020

Now rebased over #965, so potentially good to go. @samoht, @liautaud: reviews welcome.

in

(* Test cases for basic types *)
test "unit" T.unit "Prim Unit";
Copy link
Member

Choose a reason for hiding this comment

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

Is Prim really necessary here? Maybe the printer should just omit these.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed 🙂 I've included this in my list of suggestions in the PR description.

I think it's good to test the existing behaviour first.

Copy link
Member

Choose a reason for hiding this comment

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

Hum yes I jumped directly to the code without reading the description, feel free to ignore my comments ;-)

test "array {size=3}" T.(array ~len:(`Fixed 3) unit) "Array:<3> (Prim Unit)";

(* Test cases for algebraic combinators *)
test "enum" Algebraic.my_enum_t "Variant";
Copy link
Member

@samoht samoht Mar 13, 2020

Choose a reason for hiding this comment

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

Do we want to be a bit more precise for variants and records? I guess these printers are just for debugging anyway, but we could imagine extending them one day to become proper schemas, and having a precise printer would help parsing. I agree this is clearly not very urgent to do :-)

@samoht samoht merged commit d5661ba into mirage:master Mar 14, 2020
@samoht
Copy link
Member

samoht commented Mar 14, 2020

Thanks! Let's do the improvement in new PRs as the current one already improved the current situation quite a bit.

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.

irmin.type: test pp_ty for all generic combinators
2 participants