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

Incorrect async indentation when inside of a match #2501

Closed
3 tasks
MangelMaxime opened this issue Sep 14, 2022 · 6 comments · Fixed by #2527
Closed
3 tasks

Incorrect async indentation when inside of a match #2501

MangelMaxime opened this issue Sep 14, 2022 · 6 comments · Fixed by #2527

Comments

@MangelMaxime
Copy link

Issue created from fantomas-online

Code

let x =
    match packageRegistrationUrl with
    | Some packageRegistrationUrl ->

        async {

            let response = ""

            return ""
                
        }

    | None -> failwith "Package registration url not found"

Result

let x =
    match packageRegistrationUrl with
    | Some packageRegistrationUrl ->

      async {

          let response = ""

          return ""

      }

    | None -> failwith "Package registration url not found"

Problem description

The async { is not indented using 4 spaces but 2 spaces only.

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.

Options

Fantomas master branch at 2022-09-11T16:37:09Z - 7fb34e5

    { config with
                MaxLineLength = 80
                MaxRecordWidth = 0
                MaxArrayOrListWidth = 0
                MultilineBlockBracketsOnSameColumn = true
                MultiLineLambdaClosingNewline = true
                BarBeforeDiscriminatedUnionDeclaration = true
                ExperimentalStroustrupStyle = true
                KeepMaxNumberOfBlankLines = 1 }

Did you know that you can ignore files when formatting from fantomas-tool or the FAKE targets by using a .fantomasignore file?

@nojaf
Copy link
Contributor

nojaf commented Sep 14, 2022

Hey there,

Well, this opens a wonderful new can of worms😔
If there is trivia before the Stroustrup expression it should not start printing the code at the current line.

So, without the addition newline you would have something like:

| Some packageRegistrationUrl ->
    async {

formatted to

| Some packageRegistrationUrl -> async {

I assume Maxime you added the new line deliberately?

//cc @josh-degraw

@MangelMaxime
Copy link
Author

Hello,

I think I added the new line on purpose yes. The code where, this bug occurred to me is pretty dense:

image

Adding the new line, made it a bit easier to read it.

@josh-degraw
Copy link
Contributor

@nojaf What's do you think will the best way to handle this? Could we add a check inside e.g. autoIndentAndNlnExpressUnlessStroustrup to alternate based on if there is trivia before? Or would it have to happen more locally to the expression type? I see lastWriteEventIsNewline but I'm not sure if that's all I'd need or if I'd need to do more manual inspection of the trivia.

@nojaf
Copy link
Contributor

nojaf commented Sep 15, 2022

Well, I'm afraid the problem is two folded:

  • It can happen if there is trivia after the anchor (-> in this case, sample)
  • And occurs if there is trivia before the Stroustrup SynExpr.

We might be able to check these conditions in autoIndentAndNlnExpressUnlessStroustrup, but that wouldn't cover all there is to cover I think.

@MangelMaxime, do note that adding the additional newline deliberately is forcing Fantomas to behave inconsistently. If you are working in a team, another team member would not have made this decision, leading to the entire codebase being a mixed bag.

@josh-degraw
Copy link
Contributor

josh-degraw commented Sep 20, 2022

@nojaf This probably merits discussion in the style guide repo, but when formatting my team's codebase it brought me back to this issue.

I wonder if for Stroustrup we actually should either always put computation expression names on their own line, or have an option to prefer that. I'd actually lean towards the first at the moment to be honest.

I personally think that this:

let private extractMissingHeaders (error: CsvHelper.HeaderValidationException) =
    seq {
        for m in Regex.Matches (error.Message, "Header with name '([a-zA-Z]+)", RegexOptions.None) do
            yield m.Groups.[1].Value
    }

is more readable than this:

let private extractMissingHeaders (error: CsvHelper.HeaderValidationException) = seq {
    for m in Regex.Matches (error.Message, "Header with name '([a-zA-Z]+)", RegexOptions.None) do
        yield m.Groups.[1].Value
}

But, aside from the personal preference, there's a logical reason to prefer the first as well.

Stroustrup is about brackets, whereas this brings the first line of the whole expression to the first line. Changing to the first form here still has the stroustrup style brackets, but keeps the expression itself on its own line.

@nojaf
Copy link
Contributor

nojaf commented Sep 20, 2022

I had similar thoughts on this, let's discuss this further on Thursday.

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.

3 participants