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

Feature request: provide option for begin to align with end #2438

Open
smasher164 opened this issue Sep 15, 2023 · 7 comments
Open

Feature request: provide option for begin to align with end #2438

smasher164 opened this issue Sep 15, 2023 · 7 comments

Comments

@smasher164
Copy link

Given this snippet:

match x with
  | EProj (rcd, fld) -> (
      match infer' env rcd with
      | TRecord (name, rec_ty) -> (
          match List.assoc_opt fld rec_ty with
          | Some ty -> ty
          | _ -> raise (missing_field fld name))
      | ty -> raise (type_error "TRecord" (ty_kind ty)))

if i format this after setting exp-grouping = preserve, and change the parens to begin/end, I see

match x with
  | EProj (rcd, fld) -> begin
      match infer' env rcd with
      | TRecord (name, rec_ty) -> begin
          match List.assoc_opt fld rec_ty with
          | Some ty -> ty
          | _ -> raise (missing_field fld name)
        end
      | ty -> raise (type_error "TRecord" (ty_kind ty))
    end

Because the begin is not aligned with the end, it is awkward to see how the end is supposed terminate the match. There is this form that occurs.

  match
  |
  |
end

Describe the solution you'd like
Provide the ability to require that the begin align with the end, so it can look something like this:

match x with
  | EProj (rcd, fld) ->
      begin match infer' env rcd with
      | TRecord (name, rec_ty) ->
          begin match List.assoc_opt fld rec_ty with
          | Some ty -> ty
          | _ -> raise (missing_field fld name)
          end
      | ty -> raise (type_error "TRecord" (ty_kind ty))
      end
@gpetiot
Copy link
Collaborator

gpetiot commented Sep 15, 2023

@Julow do you remember if this formatting was intentional? IIRC the reason would be that switching the value of exp-grouping back and forth would not cause a massive diff. In this proposal the diff is not too bad (the ) that changed into end already moved to the next line so we can accept similar changes?)

@Julow
Copy link
Collaborator

Julow commented Sep 18, 2023

IIRC the reason would be that switching the value of exp-grouping back and forth would not cause a massive diff.

I think @smasher164 changed himself the parens into begin/end.

The begin/end might be considered to logically go with the nested match, and indeed that looks weird:

                        begin
      match
      |
      |
    end

But we can also consider that the begin/end go with the match case, and that looks a lot more reasonnable:

| EProj (rcd, fld) -> begin
    <expr>
  end

This is the way match cases are parenthesized and it's intended.

I think you are asking for something else: an option to specially parenthesize matches.

@smasher164
Copy link
Author

@Julow I think the suggested solution is parenthesized the same. The left-paren / begin follows the -> in the first pattern, and likewise with the second. The only difference is that the left-paren (begin in this case) is on a separate line, aligned horizontally with the end. If it is possible to make ocamlformat lenient when it sees begin and end vertically aligned, that would be great.

But we can also consider that the begin/end go with the match case

This is a valid point. However, the | aligned to the right of the end, and the vertical gap imposed by <expr> make it hard to notice your intention here, that the pattern and end are aligned together.

@Julow
Copy link
Collaborator

Julow commented Sep 18, 2023

the vertical gap imposed by make it hard to notice your intention here

I don't think that's true. Nothing else can be indented that much in a case, the end is clearly part of the pattern line.
It's either one of these, depending on the nature of <expr>:

| EProj (rcd, fld) ->
    <expr>

| EProj (rcd, fld) -> begin
    <expr>
  end

However, this is bad:

| EProj (rcd, fld) -> begin
    <expr>
end

The end has the same indentation as an other pattern and looks like it closes the entire match, like in your example where the begin is before the keyword match.

But what you want is for the begin/end to not be specially handled by the case but instead be formatted as part of the expression ?

| EProj (rcd, fld) ->
    begin <expr>
    end

@smasher164
Copy link
Author

but instead be formatted as part of the expression ?

Yes, It'd be great it if there were an option that allowed this formatting. I find it easier to follow nested matches when the begin and end are aligned together. So even if ocamlformat didn't move the begin to the right-hand-side of the ->, if it was previously formatted as part of the expression, that'd be fine.

Thanks

@Julow
Copy link
Collaborator

Julow commented Sep 18, 2023

Unfortunately, there's no option for this. The formatting of begin/end inside a match case is made here: https://github.com/ocaml-ppx/ocamlformat/blob/main/lib/Params.ml#L216

@smasher164 smasher164 changed the title Feature request: require begin to align with end Feature request: provide option for begin to align with end Sep 18, 2023
@yawaramin
Copy link

I think begin at the end of the line, after non-whitespace characters, should jump down to the following line and align with the end:

(* no *)
| EProj (rcd, fld) -> begin
    <expr>
  end

(* yes *)
| EProj (rcd, fld) ->
  begin
    <expr>
  end

But begin followed by some non-whitespace characters on the line should stay where it is:

Switch.run begin fun sw ->
  traceln "In switch"
end;
traceln "Switch is finished"

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.

4 participants