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

Fix the format idempotency of open type declarations #1794

Merged

Conversation

barkmadley
Copy link
Contributor

e.g.

  type a = b = ..;

was being formatted as

  type a = .. = b;

e.g.

  type a = b = ..;

was being formatted as

  type a = .. = b;
@jordwalke
Copy link
Member

You are at it again!

@jordwalke jordwalke merged commit 8a4cb68 into reasonml:master Jan 29, 2018
@jordwalke
Copy link
Member

I assume you ran into this issue in a real world app. How are you pulling in your custom fixes into your personal project so that these don't become blockers for you?

@barkmadley
Copy link
Contributor Author

To be honest, I'm not even sure what the form type x = ..; is used for, I've never had to reach for the feature myself in my OCaml work.

I just decided to increase the unit test code coverage for the reason_pprint_ast.ml file. I integrated bisect into my local dev build and am slowly going through the uncovered lines. So far I have 3 other test cases that increase coverage but this one uncovered an actual bug so I thought I wouldn't wait to create a PR.

@barkmadley barkmadley deleted the open-public-named-type-idempotent branch January 29, 2018 21:16
@jordwalke
Copy link
Member

That's awesome. Thanks for helping us improve the quality of the code.

I'm curious how you are finding code that is not executed in the formatting tests. Is there an OCaml utility you are using?

@jordwalke
Copy link
Member

By the way type t = ..; is a cool feature called extensible variants. It allows one place to define the start of a type, and then allows multiple other modules that depend on it, to extend that type - without any of the extenders having to know about each other. It's a great utility for implementing plugin systems that can inject themselves into a system, where all the plugins don't need to know about each other.

@barkmadley
Copy link
Contributor Author

Ah so thats how extensible variants are defined. Back in my day we wrote our plugin systems with first class modules. It worked pretty well.

To get code coverage reports I

  1. opam install bisect_ppx
  2. add the (preprocess (pps (bisect_ppx))) line to the reason-parser/jbuild file:

screenshot from 2018-01-30 11-46-11

  1. make test (jbuilder will rebuild everything correctly which is awesome) this generates a bunch of bisect_XXXX.out
  2. generate a coverage report using bisect-ppx-report -ignore-missing-files -I _build/ -html coverage/ bisect*.out ./*/*/*/bisect*.out (and then I usually clear out the bisect.out files using a find/rm command.

The report looks like this:
screenshot from 2018-01-30 11-45-38

I will probably add a makefile target to make this process a bit easier. At the moment I'm relying on shell history to not forget anything.

@jordwalke
Copy link
Member

I would definitely use such a makefile target.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants