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

Misplaced comment in "else if" #3011

Open
4 tasks
Smaug123 opened this issue Dec 6, 2023 · 3 comments
Open
4 tasks

Misplaced comment in "else if" #3011

Smaug123 opened this issue Dec 6, 2023 · 3 comments

Comments

@Smaug123
Copy link
Contributor

Smaug123 commented Dec 6, 2023

Issue created from fantomas-online

Code

let foo =
    if bar then
        a <- 1
    else
    // whatnot
    if baz then
        quux <- 3

Result

let foo =
    if bar then
        a <- 1
    else if
        // whatnot
        baz
    then
        quux <- 3

Problem description

The comment has been moved with respect to the if statement. Expected instead was:

let foo =
    if bar then
        a <- 1
    else
        // whatnot
        if baz then
            quux <- 3

(or, in my own codebase with ExperimentalKeepIndentInBranch, I expected no change to the formatting).

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 2023-12-06T16:47:58Z - 0f8ee23

Default Fantomas configuration

Did you know that you can ignore files when formatting by using a .fantomasignore file?
PS: It's unlikely that someone else will solve your specific issue, as it's something that you have a personal stake in.

@nojaf
Copy link
Contributor

nojaf commented Dec 7, 2023

So, the style guide settled on formatting multiline conditions like:

// ✔️ OK, but better to refactor, see below
if
    complexExpression a b && env.IsDevelopment()
    || someFunctionToCall
        aVeryLongParameterNameOne
        aVeryLongParameterNameTwo
        aVeryLongParameterNameThree 
then
        e1
    else
        e2

// ✔️The same applies to nested `elif` or `else if` expressions
if a then
    b
elif
    someLongFunctionCall
        argumentOne
        argumentTwo
        argumentThree
        argumentFour
then
    c
else if
    someOtherLongFunctionCall
        argumentOne
        argumentTwo
        argumentThree
        argumentFour
then
    d

// whatnot between the else and the if forces this behaviour.

It rings a bell that putting anything between the else if can very easily lead to invalid code, so I believe we deliberately moved the comment.

From a technical point of view, we view your entire if expression as one node, and not the parent if/else node where the else expression is a if/then expression.

@majocha
Copy link
Contributor

majocha commented Dec 7, 2023

I think it would be more logical to assign the comment to the else if node rather than to the condition, i. e. this:

if bar then
    a <- 1
else
    // elif comment
    if
        // baz comment
        baz 
    then
        quux <- 3

should not format to:

if bar then
    a <- 1
else if
    // elif comment
    // baz comment
    baz
then
    quux <- 3

but instead:

if bar then
    a <- 1
// elif comment
else if
    // baz comment
    baz
then
    quux <- 3

fantomas-online repro

@nojaf
Copy link
Contributor

nojaf commented Dec 7, 2023

Feel free to investigate how much fun that would be 😉

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

No branches or pull requests

3 participants