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

Opening parens make unstable/incorrect indentation for an Enum wrapping a record #2943

Closed
2 of 4 tasks
pbiggar opened this issue Aug 7, 2023 · 5 comments · Fixed by #2946
Closed
2 of 4 tasks

Opening parens make unstable/incorrect indentation for an Enum wrapping a record #2943

pbiggar opened this issue Aug 7, 2023 · 5 comments · Fixed by #2946

Comments

@pbiggar
Copy link
Contributor

pbiggar commented Aug 7, 2023

Issue created from fantomas-online

Code

((Combo { e1 = "Making this long so it goes on a new line new line new line new line making it long so it goes on a new line new line" }))

Result

((Combo

  { e1 =
      "Making this long so it goes on a new line new line new line new line making it long so it goes on a new line new line" }))

Expected Result

((Combo
    { e1 =
        "Making this long so it goes on a new line new line new line new line making it long so it goes on a new line new line" }))

Problem description

AFAICT, this only happens when we have 2 parens around this, which happened in my original example because I was piping the result.

Extra information

  • The formatted result breaks my code.
  • The formatted result gives compiler warnings.
  • I or my company would be willing to help fix this.
  • I would like a release if this problem is solved.

Options

Fantomas main branch at 1/1/1990

    { config with
                IndentSize = 2 }
@nojaf
Copy link
Contributor

nojaf commented Aug 16, 2023

Hi Paul, This is an interesting case. Thanks for reporting it!

My first impression is that the parentheses turn the application into a sequential expression.
Something where ((a b)) becomes

((a
  b))

Where b is no longer applied to a but the two are indidual expressions in a sequence.
I'm able to replicate this with indent = 4 and 4 parentheses.
The additional newline is introduced because the b is multiline and that follows the newline heuristic.

I'm not sure what the best way would be to solve this one. Changing this behaviour will for sure break something else.

@pbiggar
Copy link
Contributor Author

pbiggar commented Aug 16, 2023

I'm not sure I follow. Are you saying that ((a b)) is semantically the same as

a
b

That seems weird to me but it could be the case.

@nojaf
Copy link
Contributor

nojaf commented Aug 16, 2023

Oh no, it isn't and that is part of the problem.
b at least needs one space (I think) before it is considered applied to a instead of being two expressions (that are not ignored).

@pbiggar
Copy link
Contributor Author

pbiggar commented Aug 18, 2023

Oh right, that's what I thought. I got confused that changing the behaviour will break something else - what are you thinking of there?

@nojaf
Copy link
Contributor

nojaf commented Aug 18, 2023

Well, this is tricky as there a multiple places where we deal with parentheses.
But right now we do:

(a
    b)

Where b is indented from the point of view of the (, following the normal flow.
b will be placed at one indent_size unit.

I initially was thinking of maybe touching this and having b indent from a offset instead.
This would of course mean one indent_size + whatever offset a currently has.
The trade-off is that b is put on at column which won't be a multitude of the selected indent_size.
This upsets a lot of people of course.

Now, we might want to have a sanity check on the formatting of the application.
If we could assert that the arguments are at least one space further we could add an additional indent if that is not the case. The downside here is that applications are also covered in a lot of places.
Meaning, that we might be able to fix the problem at hand but it might surface again in a slightly different code sample.

I took a stab at this idea at #2946
I'll probably consult @dawedawe and others next week on this one as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants