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

Why is patdiff instructed to show unrefined diffs? #4254

Closed
hakuch opened this issue Feb 18, 2021 · 3 comments · Fixed by #4257
Closed

Why is patdiff instructed to show unrefined diffs? #4254

hakuch opened this issue Feb 18, 2021 · 3 comments · Fixed by #4257

Comments

@hakuch
Copy link
Contributor

hakuch commented Feb 18, 2021

Desired Behavior

When it's installed, Dune uses the patdiff utility to show diffs as part of, for example, expectation tests.

My understanding is that the value proposition of patdiff is that it shows more refined diffs than whole-line changes.

For example, a diff produced by patdiff could identify clearly a single integer being different on a line that's otherwise the same.

Current behavior

While there's an explicit check for patdiff being present here, the -unrefined flag is being passed to patdiff, which is counter-intuitive to me.

Is the reason for this performance? If so, I'd personally prefer that the default was to omit -unrefined and to make its inclusion an option exposed somehow in Dune.

@ghost
Copy link

ghost commented Feb 18, 2021

That's a good question... I dig into the history and this argument was passed since the beginning. Seems like I just copy&pasted at the beginning from the way we were calling patdiff in ppx_expect. We should probably remove it.

@hakuch
Copy link
Contributor Author

hakuch commented Feb 18, 2021

It's always a pleasure to know I asked a helpful question! 😃

Could I take a stab at fixing this with a PR? It might be a nice way to learn a little more about Dune development.

@ghost
Copy link

ghost commented Feb 22, 2021

Definitely, thanks for asking and making a PR :)

rgrinberg added a commit to rgrinberg/opam-repository that referenced this issue Mar 7, 2021
…ne-action-plugin, dune-private-libs and dune-glob (2.8.3)

CHANGES:

- Make `patdiff` show refined diffs (ocaml/dune#4257, fixes ocaml/dune#4254, @hakuch)

- Fixed a bug that could result in needless recompilation under Windows due to
  case differences in the result of `Sys.getcwd` (observed under `emacs`).
  (ocaml/dune#3966, @nojb).

- Restore compatibility with Coq < 8.10 for coq-lang < 0.3 , document
  that `(using coq 0.3)` does require Coq 8.10 at least (ocaml/dune#4224, fixes
  ocaml/dune#4142, @ejgallego)

- Add a META rule for 'compiler-libs.native-toplevel' (ocaml/dune#4175, @AltGr)

- No longer call `chmod` on symbolic links (fixes ocaml/dune#4195, @dannywillems)

- Dune no longer automatically create or edit `dune-project` files
  (ocaml/dune#4239, fixes ocaml/dune#4108, @jeremiedimino)

- Have `dune` communicate the location of the standard library directory to
  `merlin` (ocaml/dune#4211, fixes ocaml/dune#4188, @nojb)

- Workaround incorrect exception raised by Unix.utimes (OCaml PR#8857) in
  Path.touch on Windows (ocaml/dune#4223, @dra27)

- `dune ocaml-merlin` is now able to provide configuration for source files in
  the `_build` directory. (ocaml/dune#4274, @voodoos)

- Automatically delete left-over Merlin files when rebuilding for the first time
  a project previously built with Dune `<= 2.7`. (ocaml/dune#4261, @voodoos, @aalekseyev)

- Fix `ppx.exe` being compiled for the wrong target when cross-compiling
  (ocaml/dune#3751, fixes ocaml/dune#3698, @toots)

- `dune top` correctly escapes the generated toplevel directives, and make it
  easier for `dune top` to locate C stubs associated to concerned libraries.
  (ocaml/dune#4242, fixes ocaml/dune#4231, @nojb)

- Do not pass include directories containing native objects when compiling
  bytecode (ocaml/dune#4200, @nojb)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant