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 an auto-formatter for dune files #940

Closed
emillon opened this issue Jul 2, 2018 · 11 comments · Fixed by ocaml/opam-repository#12622
Closed

Add an auto-formatter for dune files #940

emillon opened this issue Jul 2, 2018 · 11 comments · Fixed by ocaml/opam-repository#12622
Assignees

Comments

@emillon
Copy link
Collaborator

emillon commented Jul 2, 2018

As discussed before, an autoformatter for dune files would be great to remove some microdecisions for maintainers.

It seems that Jane Street has some code for this, but it's not released yet and has external dependencies, so it might not be ideal in a first step.

@rgrinberg
Copy link
Member

As said in the previous thread, we have enough pretty printed code not to worry adding a little more here to properly format sexps. Thinking about the immediate problems posed here:

  • Do we only stick to formatting dune files or also jbuild files?

  • We need a parser that doesn't discard comments and preserves string wrapping. Since I assume our formatter will want to preserve those.

Here's another suggestion I have for this command: do not add any configurability of the formatting.

@emillon
Copy link
Collaborator Author

emillon commented Jul 2, 2018

I agree about the non-configurability part.

@hcarty
Copy link
Member

hcarty commented Jul 2, 2018

As a user, dune-only formatting seems acceptable given that jbuilder has a limited life ahead of it.

@rgrinberg
Copy link
Member

Actually, I just remembered that dune might get a parser generator eventually. Maybe it makes sense for it to also include an auto formatter? Avoid this issue entirely.

@ghost
Copy link

ghost commented Jul 3, 2018

I don't know when that will land though. Formatting s-expressions is simpler than the parsing part, we could start with a generic s-expression printer. The only issue if we format s-expressions without knowing the grammar is that we might not produce the best formatting possible.

We could also extract the grammar from the Of_sexp.t if we made it an applicative. However it's really painful to write applicative code with let%map.

@emillon
Copy link
Collaborator Author

emillon commented Jul 3, 2018

I wonder how far we can go with just a "loose list of keywords" rather than a fully fledged parser. A nice side effect is that once we have that list, we can use it for syntax highlighting as well (I wrote this list by hand and it turns out that it's pretty incomplete).

However it's really painful to write applicative code with let%map.

I hope that most of these can be hidden behind Combinators.pair, Combinators.list etc and not have too much code using directly the applicative interface, but that's hard to tell (it's probably a good idea to use these combinators anyway).

@ghost
Copy link

ghost commented Jul 4, 2018

I wonder how far we can go with just a "loose list of keywords" rather than a fully fledged parser. A nice side effect is that once we have that list, we can use it for syntax highlighting as well (I wrote this list by hand and it turns out that it's pretty incomplete).

I suppose we can go quite far. The only risk is that with s-expressions things that are keyword can also be plain data depending on the context. It's also more work to keep everything in sync.

I hope that most of these can be hidden behind Combinators.pair, Combinators.list etc and not have too much code using directly the applicative interface, but that's hard to tell (it's probably a good idea to use these combinators anyway).

The main issue is when you need to bind many values. For instance parsing record would look like:

record (field "a" ... @> field "b" ... @> field "c" @> nil) (fun a b c -> ...)

and you have to mentally bind the various field ... to the variables. For big records it's a huge pain.

I wanted to avoid using too high-techy stuff in Dune such as ppx rewriters, but for writing applicative code it might be justified.

@rgrinberg
Copy link
Member

I'd prefer to avoid using ppx as well especially since it solves only one of many of the problems that are caused by our home grown parsing technology. A good parser generator would solve the syncing issues, formatting, and provide a much better basis for editor integration. So I think that resorting to some hacks to have a working formatter is fine for now. Certainly seems better than our own ppx stack in dune as well.

@ghost
Copy link

ghost commented Jul 4, 2018

It's a trade-off. Code generators are always much more painful to work on than direct code. Both a ppx rewriter or a parser/printer generator are code generators, but with let%map you can limit much more the amount of code that goes into the generator itself. A ppx rewriter for let%map is easier to develop and maintain.

Regarding the editor integration, aren't ppx rewriters now relatively well integrated with the editor? If we have a custom parser/printer on the other hand, it will take a custom syntax as input that will have no editor support at all.

The ideal situation would be to add let%map to OCaml, then we would just need a compatibility ppx rewriter.

@rgrinberg
Copy link
Member

Regarding the editor integration, aren't ppx rewriters now relatively well integrated with the editor? If we have a custom parser/printer on the other hand, it will take a custom syntax as input that will have no editor support at all.

I meant generating editor integration for the target language. E.g. syntax highlighting files, auto completion databases, etc for the target language. Of course if the parser generator is self hosting, then we'll have both variants editor integration :)

@ghost
Copy link

ghost commented Jul 4, 2018

I see. We can do all that if we make Of_sexp.t be an applicative. It's the same idea as cmdliner.

emillon added a commit that referenced this issue Aug 17, 2018
This is a first draft with three main limitations:

- it is language agnostic, so it does not know about field names
- it is not able to parse comments
- it does not break long lines

The formatting rules are pretty simple:

- lists composed only of atoms, quoted strings, templates, and
  singletons are displayed on a single line
- other lists are displayed with a line break after each element
- an empty line is inserted between toplevel stanzas

The CLI is pretty light: it can either read a file or standard input,
and fix a file in place. In addition, the command is named
`unstable-fmt` for now, until some guarantees are given.

Closes #940

Signed-off-by: Etienne Millon <me@emillon.org>
emillon added a commit that referenced this issue Aug 17, 2018
This is a first draft with three main limitations:

- it is language agnostic, so it does not know about field names
- it is not able to parse comments
- it does not break long lines

The formatting rules are pretty simple:

- lists composed only of atoms, quoted strings, templates, and
  singletons are displayed on a single line
- other lists are displayed with a line break after each element
- an empty line is inserted between toplevel stanzas

The CLI is pretty light: it can either read a file or standard input,
and fix a file in place. In addition, the command is named
`unstable-fmt` for now, until some guarantees are given.

Closes #940

Signed-off-by: Etienne Millon <me@emillon.org>
rgrinberg added a commit to rgrinberg/opam-repository that referenced this issue Sep 14, 2018
CHANGES:

- Ignore stderr output when trying to find out the number of jobs
  available (ocaml/dune#1118, fix ocaml/dune#1116, @diml)

- Fix error message when the source directory of `copy_files` does not exist.
  (ocaml/dune#1120, fix ocaml/dune#1099, @emillon)

- Highlight error locations in error messages (ocaml/dune#1121, @emillon)

- Display actual stanza when package is ambiguous (ocaml/dune#1126, fix ocaml/dune#1123, @emillon)

- Add `dune unstable-fmt` to format `dune` files. The interface and syntax are
  still subject to change, so use with caution. (ocaml/dune#1130, fix ocaml/dune#940, @emillon)

- Improve error message for `dune utop` without a library name (ocaml/dune#1154, fix
  ocaml/dune#1149, @emillon)

- Fix parsing `ocamllex` stanza in jbuild files (ocaml/dune#1150, @rgrinberg)

- Highlight multi-line errors (ocaml/dune#1131, @anuragsoni)

- Do no try to generate shared libraries when this is not supported by
  the OS (ocaml/dune#1165, fix ocaml/dune#1051, @diml)

- Fix `Flags.write_{sexp,lines}` in configurator by avoiding the use of
  `Stdune.Path` (ocaml/dune#1175, fix ocaml/dune#1161, @rgrinberg)

- Add support for `findlib.dynload`: when linking an executable using
  `findlib.dynload`, automatically record linked in libraries and
  findlib predicates (ocaml/dune#1172, @bobot)

- Add support for promoting a selected list of files (ocaml/dune#1192, @diml)

- Add an emacs mode providing helpers to promote correction files
  (ocaml/dune#1192, @diml)

- Improve message suggesting to remove parentheses (ocaml/dune#1196, fix ocaml/dune#1173, @emillon)

- Add `(wrapped (transition "..message.."))` as an option that will generate
  wrapped modules but keep unwrapped modules with a deprecation message to
  preserve compatibility. (ocaml/dune#1188, fix ocaml/dune#985, @rgrinberg)

- Fix the flags passed to the ppx rewriter when using `staged_pps` (ocaml/dune#1218, @diml)

- Add `(env var)` to add a dependency to an environment variable.
  (ocaml/dune#1186, @emillon)

- Add a simple version of a polling mode: `dune build -w` keeps
  running and restarts the build when something change on the
  filesystem (ocaml/dune#1140, @kodek16)

- Cleanup the way we detect the library search path. We no longer call
  `opam config var lib` in the default build context (ocaml/dune#1226, @diml)

- Make test stanzas honor the -p flag. (ocaml/dune#1236, fix ocaml/dune#1231, @emillon)

- Test stanzas take an optional (action) field to customize how they run (ocaml/dune#1248,
  ocaml/dune#1195, @emillon)

- Add support for private modules via the `private_modules` field (ocaml/dune#1241, fix
  ocaml/dune#427, @rgrinberg)

- Add support for passing arguments to the OCaml compiler via a
  response file when the list of arguments is too long (ocaml/dune#1256, @diml)

- Do not print diffs by default when running inside dune (ocaml/dune#1260, @diml)

- Interpret `$ dune build dir` as building the default alias in `dir`. (ocaml/dune#1259,
  @rgrinberg)

- Make the `dynlink` library available without findlib installed (ocaml/dune#1270, fix
  ocaml/dune#1264, @rgrinberg)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants