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

Add support for dune #3

Merged
merged 8 commits into from
Feb 18, 2023
Merged

Add support for dune #3

merged 8 commits into from
Feb 18, 2023

Conversation

Khady
Copy link
Contributor

@Khady Khady commented Feb 7, 2023

dune has the ability to format its files. Adding support through the format-dune-file command, even though it's not perfect.

I can drop the ocamlformat change if necessary, or put it in a different PR. It was easier to have it when developing with a modern ocaml (4.14)

This work is sponsored by Ahrefs.

@hhugo
Copy link
Owner

hhugo commented Feb 7, 2023

Thanks.
I've opened #4 to upgrade the codebase to new versions of its deps.
Can you rebase you commit on top and add some expect test for the dune features ? (just look at other tests for inspiration)

@hhugo
Copy link
Owner

hhugo commented Feb 15, 2023

@Khady, you should be able to rebase on master and have the CI run

Comment on lines 165 to 171
(executable
(name aaaa)
(libraries unix)
(public_name pppp))

(library
(name liblib)) |}])
Copy link
Contributor Author

@Khady Khady Feb 15, 2023

Choose a reason for hiding this comment

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

the output is not correct, it results into branch2 formatted and the content of old_branch1 is dropped. IIUC the final state should be

(executable
 (name bbbb)
 (libraries unix)
 (public_name pppp))

(library
 (name liblib))

(alias
 (name runtest)
 (action
  (diff rebase.diff rebase.diff.gen)))

@Khady
Copy link
Contributor Author

Khady commented Feb 15, 2023

another problem when running merge_fmt by hand

$ _build/default/src/merge_fmt.exe 
merge-fmt: internal error, uncaught exception:
           Invalid_argument("Filename.chop_extension")
           Raised at Stdlib.invalid_arg in file "stdlib.ml", line 30, characters 20-45
           Called from Resolve_cmd.create_tmp in file "src/resolve_cmd.ml", line 71, characters 13-44
           Called from Resolve_cmd.fix in file "src/resolve_cmd.ml", line 84, characters 13-52
           Called from Resolve_cmd.resolve.(fun) in file "src/resolve_cmd.ml", line 122, characters 26-68
           Called from Base__Map.Accessors.iteri in file "src/map.ml" (inlined), line 1794, characters 19-40
           Called from Resolve_cmd.resolve in file "src/resolve_cmd.ml", line 116, characters 2-687
           Called from Cmdliner_term.app.(fun) in file "cmdliner_term.ml", line 24, characters 19-24
           Called from Cmdliner_eval.run_parser in file "cmdliner_eval.ml", line 34, characters 37-44

@Khady Khady marked this pull request as draft February 15, 2023 05:08
@Khady
Copy link
Contributor Author

Khady commented Feb 15, 2023

and now getting another error. I can't spend more time on it right now. Will try to come back to that in the coming weeks

using this test repo for reference https://github.com/Khady/merge-fmt-test

$ merge_fmt.exe --echo
+ git ls-files -u
+ git show c27aa6cb49462bf5414fce78a204491aebff17b9
+ git show 4df152dd4dba64fe71c148934c93831b3ff4a12f
+ git show cbc79883f33bbbde24480b79ac7592d9a01f8a4e
+ dune format-dune-file -- dune >  dune.ours
File "/tmp/test-git/dune", line 11, characters 17-18:
11 |  (libraries unix))
                      ^
Error: right parenthesis without matching left parenthesis
+ dune format-dune-file -- dune >  dune.theirs
File "/tmp/test-git/dune", line 11, characters 17-18:
11 |  (libraries unix))
                      ^
Error: right parenthesis without matching left parenthesis
+ dune format-dune-file -- dune >  dune.common
File "/tmp/test-git/dune", line 11, characters 17-18:
11 |  (libraries unix))
                      ^
Error: right parenthesis without matching left parenthesis
Failed to format dune.ours, dune.theirs, dune.common
Resolved 0/1 dune
+ git ls-files -u

@hhugo
Copy link
Owner

hhugo commented Feb 15, 2023

  • I've rewrote the format command for dune files to make it more robust.
  • I've updated the test, the previous version was failing to merge.

@hhugo
Copy link
Owner

hhugo commented Feb 17, 2023

Can you add a changelog entry and mark the PR as ready

@Khady Khady marked this pull request as ready for review February 18, 2023 11:55
@hhugo hhugo merged commit 31d1d45 into hhugo:master Feb 18, 2023
@hhugo
Copy link
Owner

hhugo commented Feb 18, 2023

Thanks, should be released soon. ocaml/opam-repository#23355

hhugo pushed a commit to hhugo/opam-repository that referenced this pull request Feb 18, 2023
CHANGES:

- Add support of dune formatter. (hhugo/merge-fmt#3)
@Khady Khady deleted the louis/dune-fmt branch September 16, 2023 10:49
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.

2 participants