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

F# style guide question - multiline expression in match or if expression #26507

Open
nojaf opened this issue Oct 14, 2021 · 16 comments
Open

F# style guide question - multiline expression in match or if expression #26507

nojaf opened this issue Oct 14, 2021 · 16 comments
Labels
doc-enhancement Improve the current content [org][type][category] dotnet-fsharp/svc Pri2

Comments

@nojaf
Copy link
Contributor

nojaf commented Oct 14, 2021

Hello @dsyme ,

Following up on https://github.com/fsprojects/fantomas/issues/1915, I'd like to verify how you see the formatting of long (say multiline) expressions inside if/then/else of match/with.

I have two questions here:

  • Are the rules the same for if/then as they are for match/with? I always assumed so to remain consistent.
  • The docs for if/then/else mention
    image

What is long? Would any multiline expression fit the bill here?
Example

if directoryRouter.GetIdentity()
   |> self.ServerDescriptors.TryFind then
    CircuitNodeDetail.FastCreate
else
    self.ConvertToCircuitNodeDetail serverDescriptor

Should it be

if 
      directoryRouter.GetIdentity ()
      |> self.ServerDescriptors.TryFind then
      CircuitNodeDetail.FastCreate
else
    self.ConvertToCircuitNodeDetail serverDescriptor

?


Document Details

Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

@dsyme
Copy link
Contributor

dsyme commented Oct 14, 2021

Long is multi-line (but does it matter? the then is still always at the end?)

Should it be

No, I don't think so - I don't agree with the extra line added in the style guide

@nojaf
Copy link
Contributor Author

nojaf commented Oct 14, 2021

Long is multi-line (but does it matter?

Well, if long = multiline then the example should have the expression on the next line one scoped indented from the if.

No, I don't think so - I don't agree with the extra line added in the style guide

Which kinda contradicts the long = multiline right? Or do you mean something else?

So where does that leave Are the rules the same for if/then as they are for match/with? I always assumed so to remain consistent.?

I'm well aware I'm asking difficult questions here 😅, but under which conditions would the following apply:

if a then
    b

// what rules should apply to `a` to become

if
    a then
    b

And are these rules the same for

match a with
| _ -> b

// what rules should apply to `a` to become

match
    a
    with
| _ -> ()

One rule I can think of is when a is a try/with or match/with itself.
I know people shouldn't be doing this, but it is the mad world we live in.

@dsyme
Copy link
Contributor

dsyme commented Oct 14, 2021

I personally think we should never be inserting the newlines, so always

if multi
       line then
    b

match multi
          line with
| _ -> ()

I guess one indent from multi

But I don't mind too much. I think the with and then should still always be at the end. People should just not be doing this however

@dsyme
Copy link
Contributor

dsyme commented Oct 14, 2021

I do use

if long-thing &&
   another-long-thing  &&
   another-long-thing &&
   another-long-thing then

in the compiler. I don't think this should get any extra new-lines

@nojaf
Copy link
Contributor Author

nojaf commented Oct 15, 2021

I personally think we should never be inserting the newlines, so always
I guess one indent from multi

Which to me sounds reasonable but unfortunately contradicts https://github.com/dotnet/docs/pull/26294/files#diff-5583236f005ad44753c6945d4312c09cbd439fb251f9a01db543048fe51d284bR77-R84

@dsyme
Copy link
Contributor

dsyme commented Oct 15, 2021

Yes that example isn't demonstrating the rule bring discussed is it? I don't think it should be in the guide, I didn't think enough before merging it.

The formattings above don't suffer from renaming problems AFAICS?

Let's remove those examples from the guide?

@nojaf
Copy link
Contributor Author

nojaf commented Oct 15, 2021

Let's remove those examples from the guide?

Sounds good to me, however, I may want to add an explicit section to cover that the specific cause is not a vanity alignment.
The term is being used a lot in Fantomas issues, people need to know where the boundaries of that section lie.

On a side note, could we perhaps introduce the policy that all PR's for the style guide need a discussion in an issue first?
I feel like going a bit slower over these topics often benefits the bigger picture.

Lastly, I do want to give @knocte a chance to reply to all of this as he made the previous PR and has an interest in this.

@knocte
Copy link
Contributor

knocte commented Oct 15, 2021

Let's remove those examples from the guide?

Wait a second. So then you don't agree with the principles behind this past (&closed) issue?: #21487

Because this:

I guess one indent from multi

actually means vanity indentation. Let's see the example again:

if long-thing &&
   another-long-thing  &&
   another-long-thing &&
   another-long-thing then

Am I seeing what everyone is seeing? a 3-space indentation?

Are we out of our minds here? :)

@knocte
Copy link
Contributor

knocte commented Oct 15, 2021

Lastly, I do want to give @knocte a chance

Thanks for that mention, but I want to bring my lawyer too: @cmeeren 😅

@dsyme
Copy link
Contributor

dsyme commented Oct 15, 2021

Wait a second. So then you don't agree with the principles behind this past (&closed) issue?: #21487

I'll review those discussions carefully and have a think about this. I sort of missed the significance of that dicussion and didn't participate in it.

Am I seeing what everyone is seeing? a 3-space indentation?

Painful, it's true.

That said, "Avoid vanity (e.g. 3-space) indentation" is certainly not an established principle in the style guide - it's not actually mentioned anywhere besides in that PR and I think there are existing samples in the guide that break this principle. It's quite a big thing to be adding to the guide and for all Fantomas formatting defaults.

@dsyme
Copy link
Contributor

dsyme commented Oct 15, 2021

@knocte Do you particularly care about the positioning of the then and with? That seems to be a separate issue

@knocte
Copy link
Contributor

knocte commented Oct 18, 2021

Do you particularly care about the positioning of the then and with?

Well, I kinda care, but agree it might be a separate issue. Thing is, while I would like to have a consistent view about where to put then and with, I notice the following:

  • When dealing with a long match line, I prefer with to be in the next line (because it's more visually appealing, and because it doesn't risk reaching MaxLineLength).

Example:

match
    veryLoooooooooooooongCondition
        foo
        bar
    with // here looks better, because if we place it next to `bar` it could be confused as another arg (or an arg of `bar`)
| FirstAlternative -> ...
  • However, I notice that desiring the same analogous behaviour in an if-then(else) block, it has more controversial consequences.

Example:

if veryLoooooooooooooongCondition
    then // already 1 level of indentation, so...
        doSomething() // 2 level of indentation here then?

AFAIU having two levels of indentation in an if-then block seems overkill, that's why in this case I prefer the then in the same line as the if.

But let's go back to the most important topic at hand: 3-spaces indentation. Cannot this code...:

if long-thing &&
   another-long-thing  &&
   another-long-thing &&
   another-long-thing then
    doSomething()

...just use a standard indentation level if the line has to be split? like this:

if long-thing &&
    another-long-thing  &&
    another-long-thing &&
    another-long-thing then
    doSomething()

This compiles, right?

Sure, I understand that it wouldn't be consistent with the following match case:

match
    long-thing &&
        another-long-thing  &&
        another-long-thing &&
        another-long-thing
    with
| FirstAlternative -> ...

However, IMO it's well justified simply because the word match is longer than the word if, and if you address both if and match cases in the same way, you're going to get things that look a bit funky (or using vanity alignment).

@dsyme
Copy link
Contributor

dsyme commented May 26, 2022

I'm coming around to the point-of-view that the default formatting and style-guide recommendation for multi-line inputs to match should be like this:

match
    directoryRouter.GetIdentity ()
    |> self.ServerDescriptors.TryFind
with
| None ->
    CircuitNodeDetail.FastCreate
| Some serverDescriptor ->
    self.ConvertToCircuitNodeDetail serverDescriptor

Is there any reason not to align the with under the match?

@knocte
Copy link
Contributor

knocte commented May 26, 2022

Is there any reason not to align the with under the match?

Yes, it doesn't compile (gives error FS0010). Not sure if this compiles with newer F# though.

@dsyme
Copy link
Contributor

dsyme commented May 26, 2022

Yes it compiles with F# 6

@dsyme
Copy link
Contributor

dsyme commented May 26, 2022

For if-then-else, it is entirely consisitent to do this:

if
    long-thing &&
    another-long-thing  &&
    another-long-thing &&
    another-long-thing
then

though

  1. that would almost always make me rewrite the code
  2. I think the default tolerance for a "long" single-line from if .. then should be large (60, 70, 80...).

I'm trying to get a read on what's "normal" for other languages. It seems C/C# auto-formatting simply don't do any formatting of boolean logic? For JS/TS, prettier does similar to the above, e.g. here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-enhancement Improve the current content [org][type][category] dotnet-fsharp/svc Pri2
Projects
None yet
Development

No branches or pull requests

6 participants