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

Consistent indentation of fun after let #323

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Julow
Copy link
Contributor

@Julow Julow commented Mar 29, 2023

Apply the same indentation for:

let f =
  fun x ->
  y

and:

let _ =
  let f =
    fun x ->
    y
  in
  ()

The reason for this change is that a top-level-let can be interpreted
as a LetIn, for example:

module M = struct
  let indentation_after_fun =
    fun foo ->
    bar
end

We found this problem while working on OCamlformat for Janestreet.
An other solution to this problem would be to correctly interpret the let. What do you think ?

Apply the same indentation for:

    let f =
      fun x ->
      y

and:

    let _ =
      let f =
        fun x ->
        y
      in
      ()

The reason for this change is that a top-level-`let` can be interpreted
as a `LetIn`, for example:

    module M = struct
      let indentation_after_fun =
        fun foo ->
        bar
    end
@AltGr
Copy link
Member

AltGr commented Mar 30, 2023

Thanks! I don't have a strong opinion about the change (although I do like the fact that the whole body doesn't change indentation depending on whether there is a newline before the fun) — but about this part:

The reason for this change is that a top-level-let can be interpreted as a LetIn, for example:

That sounds like a bug that could be fixed, could you have a look at:
https://github.com/OCamlPro/ocp-indent/pull/323/files#diff-d2b011b2196f3fb28d20777c036018fd9b3073c4211cfb45440815d778a987bbR942-R947

Thanks!

@Julow
Copy link
Contributor Author

Julow commented Apr 3, 2023

I fixed the detection of Let in #324 but I think this PR also make sense as it fixes an inconsistency.

@ceastlund
Copy link

We've looked at this change on our code at Jane Street, the ouput looks fine and more consistent than before. I'm in favor of this change.

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.

3 participants