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

Improve: disable ocamlformat if no dot ocamlformat in the project #391

Merged
merged 16 commits into from
Sep 8, 2018

Conversation

hhugo
Copy link
Collaborator

@hhugo hhugo commented Sep 3, 2018

The PR adds a way to not reformat files that live in a "project" that doesn't contain a .ocamlformat file.

@hhugo
Copy link
Collaborator Author

hhugo commented Sep 3, 2018

@samoht, @gpetiot, @jberdine, alternative suggestion welcome.

@jberdine
Copy link
Collaborator

jberdine commented Sep 4, 2018

IIUC, there is a similar situation with dune, which has resulted in a way of specifying subdirs to ignore. This would result in putting a config file in the dir containing the project root dir to be ignored, and adding that dir's name into the config. I'm not sure if that is better, but think that the space was explored at some length for dune.

@hhugo
Copy link
Collaborator Author

hhugo commented Sep 4, 2018

Just to be clear, the issue that this PR is trying to solve is about not formatting files that are not meant to be formatted. In particular, I don't want files from the ocaml compiler to be formatted when I edit them in emacs.

@jberdine
Copy link
Collaborator

jberdine commented Sep 4, 2018

I see. I guess perhaps I should enable format-on-save myself since it seems most people use it that way.

For that particular use case, you prefer to call ocamlformat and have it detect that it's disabled, rather than e.g. configuring emacs to not call ocamlformat in the first place? I'm thinking of for instance enabling format-on-save in tuareg-mode-hook only if the file name matches some user-specific whitelist.

This PR basically says to look for an .ocamlformat file in the ancestor directories of the input file, but stop looking when reaching a "project root" dir, right? I'm not sure the implementation is the simplest, but the basic approach seems good to me. We should document it in the man page where it discusses where .ocamlformat files are searched for.

Perhaps this stop at project root mode should be the default?

@hhugo
Copy link
Collaborator Author

hhugo commented Sep 4, 2018

I'm now curious about how you use ocamlformat if not with format-on-save. Some logic could be implemented in the emacs mode, but similar logic would then need to be implemented in other editor mode a well

@jberdine
Copy link
Collaborator

jberdine commented Sep 4, 2018

I'm low tech:

(add-hook
  'tuareg-mode-hook
  (lambda ()
    (define-key merlin-mode-map (kbd "M-q") 'ocamlformat)))

@samoht
Copy link
Contributor

samoht commented Sep 5, 2018

M-q only formats the current top-level expression, right? How do you do this without --lines? But maybe the expression is computed by tuareg mode already.

@jberdine
Copy link
Collaborator

jberdine commented Sep 5, 2018

My binding in merlin-mode-map shadows tuareg's, so it formats the whole buffer.

@hhugo hhugo changed the title Improve: disable ocamlformat if no dot ocamlformat Improve: disable ocamlformat if no dot ocamlformat in the project Sep 8, 2018
@hhugo
Copy link
Collaborator Author

hhugo commented Sep 8, 2018

I've reworked the PR.
The behaviour is now triggered by --inside-project-only (better name welcome).
This flags is now passed to ocamlformat in the emacs mode.
I've also added a --print-config flag that will print config updates together with their origin (see new tests)

@hhugo
Copy link
Collaborator Author

hhugo commented Sep 8, 2018

This PR should naturally support the addition of a --root option

@hhugo
Copy link
Collaborator Author

hhugo commented Sep 8, 2018

I've reverted the change to the emacs mode for now so that the behaviour is preserved (to be discussed in another PR)

src/Conf.ml Outdated
match parse value with
| packed_value ->
if verbose then log_update ~from ~name ~value ;
Some (Ok (update config packed_value))
Copy link
Collaborator

Choose a reason for hiding this comment

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

@hhugo I think it's ok to also push update out of the handler

@hhugo
Copy link
Collaborator Author

hhugo commented Sep 8, 2018

I've implemented your suggestion regarding parse, it now returns a result.

Copy link
Collaborator

@jberdine jberdine left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

src/Conf.ml Outdated
@@ -764,6 +831,11 @@ let output =
& opt (some string) default
& info ["o"; "output"] ~doc ~docs ~docv)

let print_config =
let doc = "Print config" in
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: . at end.

@hhugo hhugo merged commit 08790d2 into ocaml-ppx:master Sep 8, 2018
@hhugo hhugo deleted the scope branch September 8, 2018 13:41
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.

4 participants