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

[style-guide] Formatting of argument lists #664

Open
Thorium opened this issue May 25, 2022 · 27 comments
Open

[style-guide] Formatting of argument lists #664

Thorium opened this issue May 25, 2022 · 27 comments

Comments

@Thorium
Copy link

Thorium commented May 25, 2022

In the current OSS projects. there exists a push of Fantomas to force implementing the coding conventions of F#, where as those coding-conventions are not complete, creating a gap which is not minded.

IMHO: The coding conventions should focus on maintainability. The intention of the original developer should come clearly visible from the code. Without any kind of statistical analysis, the computer doesn't see that intention from the source code, thus it should keep its fingers off and not auto-obfuscate the code to meet some artificial convention... anyway...

Many developers use wide-screen monitors to replace multiple monitor configuration.
So the coding conventions should adapt to this current world we live in, where the development-monitors being easily 300 characters in width, but still 50 in height.

Therefore, to try to adopt to the situation, I suggest that:

  1. Arrays with data only should be allowed to write without line-breaks. Code-lines that will not be maintained line-by-line should be allowed to be long. Stroustrup lines fsprojects/fantomas#2277

  2. Unnecessary breaking of namespaces into separate lines should be avoided. Don't break namespaces unnecessarily fsprojects/fantomas#2279

  3. Functions with many parameters should add one tab more before function body Functions with many parameters should add one tab more before function body fsprojects/fantomas#2278

  4. Mutation of an object should still be preferred as the only action on that line
    Mutable expressions should be on one line fsprojects/fantomas#2280

@Thorium Thorium changed the title Some coding conventions on long lines [style-guide] Some coding conventions on long lines May 25, 2022
@auduchinok
Copy link

Unnecessary breaking of namespaces into separate lines should be avoided.

From the syntax tree view it's not possible to distinguish a namespace from a static member or a module member access. How it should probably be formulated is try to maintain line breaks inside expressions with dot access and only split them for parts that don't fit on line with current line length settings.

Mutation of an object should still be preferred as the only action on that line

I'd say the same thing here: we should try to keep the original formatting choice for places where the style guide allows variations like this.

@dsyme
Copy link
Contributor

dsyme commented May 26, 2022

Decision on (4) is here: #663

@dsyme
Copy link
Contributor

dsyme commented May 26, 2022

On (2) I agree, ideally we should never break namespaces, I think under any circumstances. However it will be difficult to implement because namespaces are not known to fantomas. I don't know how we could implement except a list of known namespaces. And really, good code should open the namespace.

Overall I think we could say this fits under "bad code formats badly"

@dsyme
Copy link
Contributor

dsyme commented May 26, 2022

On (3) I'm torn, it's not a simple choice. I'm aware it is a realy problem in practice. There is also a major risk of being inconsistent with functions and methods taking tupled paramters.

The current guidance is here: https://docs.microsoft.com/en-us/dotnet/fsharp/style-guide/formatting#formatting-function-and-member-arguments

We must also consider the variaitons.

One indent (4-space) - current style guide

let curried:

let sillyfuncWithParams 
    parameterName1
    ignoredParameterName2 
    ignoredParameterName3 =
    ...

let tupled:

let sillyfuncWithParams 
    (
        parameterName1,
        ignoredParameterName2,
        ignoredParameterName3
    ) =
    ...

let mixed:

let sillyfuncWithParams 
    parameterName0
    (
        parameterName1,
        ignoredParameterName2,
        ignoredParameterName3
    )
    parameterName4 =
    ...

let rec curried:

let rec sillyfuncWithParams 
    parameterName1
    ignoredParameterName2 
    ignoredParameterName3 =
    ...

let rec tupled:

let rec sillyfuncWithParams 
    (
        parameterName1,
        ignoredParameterName2,
        ignoredParameterName3
    ) =
    ...

let private curried:

let private sillyfuncWithParams 
    parameterName1
    ignoredParameterName2 
    ignoredParameterName3 =
    ...

let private tupled:

let private sillyfuncWithParams 
    (
        parameterName1,
        ignoredParameterName2,
        ignoredParameterName3
    ) =
    ...

static member curried:

type C() =
    static member sillyfuncWithParams 
        parameterName1
        ignoredParameterName2 
        ignoredParameterName3 =
    ...

static member tupled:

type C() =
    static member sillyfuncWithParams 
        (
            parameterName1,
            ignoredParameterName2,
            ignoredParameterName3
        ) =
    ...

Two indent (8-space)

let curried:

let sillyfuncWithParams 
        parameterName1
        ignoredParameterName2 
        ignoredParameterName3 =
    ...

let tupled:

let sillyfuncWithParams 
        (
            parameterName1,
            ignoredParameterName2, 
            ignoredParameterName3
        ) =
    ...

let mixed:

let sillyfuncWithParams 
        parameterName0
        (
            parameterName1,
            ignoredParameterName2, 
            ignoredParameterName3
        )
        parameterName4 =
    ...

let rec curried

let rec sillyfuncWithParams 
        parameterName1
        ignoredParameterName2 
        ignoredParameterName3 =
    ...

let rec tupled

let rec sillyfuncWithParams 
        (
            parameterName1,
            ignoredParameterName2,
            ignoredParameterName3
        ) =
    ...

let private curried

let private sillyfuncWithParams 
        parameterName1
        ignoredParameterName2 
        ignoredParameterName3 =
    ...

let private tupled

let private sillyfuncWithParams 
        (
            parameterName1,
            ignoredParameterName2,
            ignoredParameterName3
        ) =
    ...

static member curried:

type C() =
    static member sillyfuncWithParams 
            parameterName1
            ignoredParameterName2 
            ignoredParameterName3 =
    ...

static member tupled:

type C() =
    static member sillyfuncWithParams
           ( 
                parameterName1,
                ignoredParameterName2,
                ignoredParameterName3
            ) =
    ...

@auduchinok
Copy link

auduchinok commented May 26, 2022

@dsyme We've been using that double-indent approach in ReSharper.FSharp and I haven't seen cases where it wouldn't work well. It always makes it very clear where a function/method/type body begins.

@dsyme
Copy link
Contributor

dsyme commented May 26, 2022

@auduchinok I've added tupled and mixed cases to the above. What do you think for static members? And is there any reason that case would be different?

@dsyme
Copy link
Contributor

dsyme commented May 26, 2022

@auduchinok basically it seems it is only curried let that's a problem for single-indentation, because of the unfortuanate exact alignment between function name and curried arguments. But of course it's the most common case as well.

The question is would we use double-indentation for all of the above? Or just the let cases? Or let the single plain let? and would we use double indentation for tupled argument lists? And mixed?

@dsyme dsyme changed the title [style-guide] Some coding conventions on long lines [style-guide] Formatting of argument lists May 26, 2022
@dsyme
Copy link
Contributor

dsyme commented May 26, 2022

I've repurposed this issue to be specifically about formatting of argument lists

@auduchinok
Copy link

auduchinok commented May 26, 2022

@auduchinok I've added tupled and mixed cases to the above. What do you think for static members? And is there any reason that case would be different?

It could be something like that (i.e. follow the expression rules and keep the opening paren on the previous line):

type C() =
    static member sillyfuncWithParams( 
            parameterName1,
            ignoredParameterName2,
            ignoredParameterName3) =
        ...

In our project I'd format it like this:

type C() =
    static member sillyfuncWithParams(parameterName1, ignoredParameterName2,
            ignoredParameterName3) =
        ...

or (to add a constructor example)

type C(parameterName1, ignoredParameterName2,
        ignoredParameterName3) =
    static member sillyfuncWithParams(parameterName1, ignoredParameterName2,
            ignoredParameterName3) =
        ...

@dsyme
Copy link
Contributor

dsyme commented May 26, 2022

It's interesting to note that currently this mixed example:

let sillyfuncWithParams
    parameterName0
    (parameterName1,
     ignoredParameterName2,
     ignoredParameterName3,
     ignoredParameterName4,
     ignoredParameterName5,
     ignoredParameterName6)
    =
    1

formats the tuple portion differently to

let sillyfuncWithParams
    (
        parameterName1,
        ignoredParameterName2,
        ignoredParameterName3,
        ignoredParameterName4,
        ignoredParameterName5,
        ignoredParameterName6
    ) =
    1

@nojaf what's the currently implemented rule here? Is treating things taking one tuple of arguments as special?

@nojaf
Copy link
Contributor

nojaf commented May 30, 2022

@dsyme I believe the mixed example never came up in the style guide before.
We only seem to add the indentation when there is a single tuple parameter.

@MangelMaxime
Copy link

Coming here from F# Slack after @nojaf request for feedbacks form the community.

Personally, I agree that breaking namespace should be avoided and I like having each argument on their own lines when the formatting kicks in for multiline as it keeps the code consistent and I find it easier to read. I can scan vertically and don't have the make jumps with eyes to different indentation levels.

What I mean by jump between different indentation levels is code like that, where you never know where you should move your eyes before hand.

type C(parameterName1, ignoredParameterName2,
        ignoredParameterName3) =
    static member sillyfuncWithParams(parameterName1, ignoredParameterName2,
            ignoredParameterName3) =
        ...

Regarding, the silly function, I personally prefer this formatting because to me it is:

  1. Each arguments is on its own line
  2. Tuples is indented one level like it is when it is the only argument of the function
let sillyfuncWithParams
    parameterName0
    (
        parameterName1,
        ignoredParameterName2,
        ignoredParameterName3,
        ignoredParameterName4,
        ignoredParameterName5,
        ignoredParameterName6
    )
    =
    1

@nojaf
Copy link
Contributor

nojaf commented Dec 1, 2023

I'm more specifically asking for feedback on (3).
(The namespace thing, is a different creature).

It came up today that it could be beneficial to visually separate the parameters and the function body. So that the header of the function is more easily identified.

Something perhaps like

let sillyfuncWithParams 
        parameterName1
        ignoredParameterName2 
        ignoredParameterName3 
    =
    longBody

@MangelMaxime
Copy link

In that case, I personally prefer to stick with one level of indentation at the time (so not making the parameters / function body) visually distinct from each other.

This is because, I feel like when there will be nested functions, it could make the code heavier to read.

To me having a two indentation level breaks the visual consistency, as now something the next scope is 1 level indentation or 2 level indentation and same for the previous scope.

Example of nested functions:

let sillyfuncWithParams 
        parameterName1
        ignoredParameterName2 
        ignoredParameterName3 
    =

    let sillyfuncWithParams 
            parameterName1
            ignoredParameterName2 
            ignoredParameterName3 
        =
        longBody


    longBody

@auduchinok
Copy link

Something perhaps like

I think it looks somewhat easier on the eyes if = is placed on the parameter line

let sillyfuncWithParams 
        parameterName1
        ignoredParameterName2 
        ignoredParameterName3 =
    longBody
    ...
    longBody
let sillyfuncWithParams 
        parameterName1
        ignoredParameterName2 
        ignoredParameterName3 =

    let sillyfuncWithParams 
            parameterName1
            ignoredParameterName2 
            ignoredParameterName3 =
        longBody
        ...
        longBody

    longBody
    ...
    longBody

@smoothdeveloper
Copy link
Contributor

Putting the comma at the beginning of the line for tupled arguments would make more sense.

I would also like very much if the line length restriction would take in account the indent level of the construct as reference, rather than the first column.

@brianrourkeboll
Copy link

Are there many existing codebases that consistently indent wrapped parameters in function declarations by 8 spaces? I don't think I've seen that before. If not, it would seem like people haven't generally found the 8-space convention worth applying by hand (or that they find the 4-space convention more readable most of the time).

I'd argue for keeping single indentation throughout. Using a single indentation for wrapped parameters after a let declaration also maintains symmetry between function declaration and function application, since we wouldn't double-indent for application.

I don't think I would normally double-indent function parameters in a declaration by hand.1 If the alignment of the parameters with the function name really looked odd or hard to read, I'd probably just avoid wrapping the parameters altogether, or rename or refactor things until wrapping was no longer necessary.

Funny enough, ocamlformat's Fit_or_vertical configuration (which corresponds to our default) does use a double indent throughout for declarations, even though—since the default indentation size there is 2 spaces—it actually causes the parameters to line up with the function name in a let-bound function:

While I actually think doubling 2 to 4 looks fine, doubling 4 to 8 somehow feels heavy-handed to me.

If double-indent here did make it into the style guide, would we also update the tooling everywhere to automatically double-indent after let fEnter? Right now, I don't think VS or Ionide actually indent at all, although it seems like they should, since indenting is required regardless of what follows (a parameter, a type annotation, an equals sign, etc.).

Footnotes

  1. Actually, I think I have done that before in a codebase where 2-space indentation was the established convention, but like the ocamlformat behavior mentioned above, 2 → 4 feels less clunky than 4 → 8.

@Thorium
Copy link
Author

Thorium commented Dec 1, 2023

Your job is to fix someone's code, but you don't have infinite amount of time to reason about it.

Which one is faster to reason about where the functionality starts?

The non-intended one?
image

or the intended one?
image

(These have the same random characters.)

@smoothdeveloper
Copy link
Contributor

@Thorium for me, out of the blue, the first one, and I think it will be different, legitimately for anyone.

Scanning for the single line with = is what my "brain" seems to do.

@Thorium
Copy link
Author

Thorium commented Dec 2, 2023

I think it will be different, legitimately for anyone.

I guess that's another vote for "no" to automatic code formatting everywhere.

@smoothdeveloper
Copy link
Contributor

@Thorium, it is a vote for a more fine tuned, no black / white, all / nothing approach.

I'd like to have something that:

  • trims end lines (and knows F# to avoid bad surprises)
  • automatically puts the commas at beginning or end of line (beginning is better IMO, despite it is not the norm)
  • helps with white space alignment for select constructs
  • etc.

but does nothing drastic:

  • forces 100% of files to have or not have a \n, all the time, forcing everyone to develop OCB for such trivial thing with no impact on readability at all
  • need to reformat lines for illusory rule about "N characters on a single line is forbidden", if you move some code from one scope to another one, everything is reformatted (this is hurting for this suggestion IMO)
  • allows both several parameters on same line, and others to be split manually without touching but splitting those on same lines if line exceeds N characters from beginning of construct (not first column)

For example, if this was part of design ethos for the formatter, more than implementation details of AST, to only think of "code touch up tool" rather than "all in" it would change a lot my perception; I know none of this is trivial, in the core of the implementation, and the choices that at least, made fantomas possible, I'm grateful for.

I don't think average F# code base that is not automatically formatted, nor due to lack of "advanced editor tooling vs other big names languages" is that unreadable either, that it warrants code formatting to be required, static analyzers that can be tailored per repo have more value to me, since the editors evolved a lot, so typing code is generally formatted well enough without a lot of care.

I'm thinking conceptually, formatters could be more conservative things, and more clever (detect if some code is grid formatted, detect if indentation in file is all over the place, leave it intact, if it is is 90% one way, reformat code that is not commited), but I'm not putting it on fantomas, fantomas is awesome disregarding my views / desires, fantomas is solid and necessary for F# ecosystem.

I faced couples of negative code reviews that were just on the surface of formatting, and carried almost 100% emotionally (this is bad for any team), or some frustration with "CI workflow in the way" using fantomas or stylecop enough to not be an "adopter by default", and seen codebases with no formatting and messed up indents everwhere, in very old languages, to appreciate more my time on the soundness of the code, and automatic formatting doesn't change this aspect as much as other aspects, even in crappiest formatted code (which gets formatted if it needs change, just campground cleaning).

What I like though, is tools like editors putting braces, indenting for me, and if formatters can be tailored for "only pay for what you use", I'd adopt more of it, I still remember being amazed by emacs and stylish haskell when I was trying this at beginning of my journey with ML (by default, in records for example, it lines all the :: type with white spaces, free of any effort).

I think fantomas will make it there also, but the community needs to grow in size, and possibly more funding, or just more people that are of the "formatter or die" but needs more things than it offers, meaning more incentivized maintainers.

For the discussion of argument list, why can't we:

  • start with one implementation (I'd vote for indenting the tuples, commas at beginning, but also very much ok with "the one which is easiest to the person who has onus of implementing it in fantomas")
  • having the implementation documented a bit, with few of the "default settings for this implementation are hardcoded here and used as if we'd fill gaps later, consistently in the initial implementation"
  • a bit of reflection / bikeshedding of naming for alternative settings upfront

so other contributors could come after and fill the gaps, as a clear process so everyone gets served, while consumers like G-Research and heavy fantomas users at least get the argument formatting processed in the most desired/desirable way?

I trust all parties to not try to mess with F# :)

Thanks for always offering psychologist consultation to me @Thorium :D

@smoothdeveloper
Copy link
Contributor

I guess that's another vote for "no" to automatic code formatting everywhere.

exactly, I think fslang-design and styleguide should not be correlated 100% to what fantomas can do today, maybe it was necessary evil (such as fantomas dog fooding in compiler) and still has value to keep a correlation, but not tight coupling.

I've contributed edits to styleguide, that keep things a bit more open-ended, than what fantomas will do, while keeping the defaults as main suggestion, and I don't think it is a loss for F# or fantomas.

"automatic code formatting everywhere" sounds like dictature, help people writing / keeping pretty code, and not be emotional or stringent about formatting choices, is a better objective, for me.

@Thorium
Copy link
Author

Thorium commented Dec 2, 2023

I think the limitation with Fantomas is that it takes apples, transforms them to bananas, then transforms bananas to apples again, claiming his new apples are better than the original ones. But any originality is lost in the process, the new apples are not the original ones. That's why it'll always be hard to transfer characteristics (or developer intention) through the process. But you can claim that software development shouldn't be artistic or fun, just all machine processed like your hamburgers. Which I'm ok with, I'm more scared of future merge-conflicts on long living branches.

@smoothdeveloper
Copy link
Contributor

that software development shouldn't be artistic or fun

it is made by humans, for humans (primarily, AFAIK), and vast majority of it is not life or mission critical to warrant "enroll into army of no fun and everyone's ego thinks the same, hail Kim Jun Fantomas Syme!" at all stages, even if it is a lot of monopoly money in consideration or deciding what to use (as this doesn't equate life or mission critical defacto).

Life is whatever people make it geared towards, and it sometimes involves automatized code formatting or emerging community consensus, pragmatic, without consideration if it makes F# "for artists" or for "purists mission critical, large team, boring only allowed" (if I had to make cliché of opposites on spectrum, spectrum which can't be annihilated ever, just expanded farther and non discriminatorily).

The guidelines and fantomas brought a lot of clarity, in same fashion that "F# code I love" talk.

I still offer psychologist consultation about bleak future of humans not able to learn due to AI being bad, to help people overcome such illusory anxiety :D

@smoothdeveloper
Copy link
Contributor

fsharp_method_parameters_

  • enforces_one_per_line_when_beyond_line_limit = true / false (default is false)
  • enforces_comma_at = beginning / end / discard_rule (default is empty / discard rule: not touching/preserving)
  • multiline_tupled_arguments_style = indent | same_level | discard_rule (default is empty / discard rule: not touching/preserving)
  • multiline_indent_level = minimum / one_more_than_body (default is minimum, due to line length semantic being a big apple to banana to apple issue)
  • multiline_equal_placement= solo / end_line_of_last_parameter

just throwing it out there, I think we should discuss on those grounds, and possibly have this as metadata that is toolable upfront, rather than "html source of of fantomas doc is the source of truth" or "many places of fantomas codebase".

Then the decision for first impl. and sole setting is implementor and sponsor driven, so long majority of community is not broadcasting alert.

@charlesroddie
Copy link
Contributor

The 8-space is much better as it distinguishes arguments from the function itself and from the implementation.
We use this with starting bracket on the same line, exactly as in #664 (comment) .

@Lanayx
Copy link

Lanayx commented Dec 4, 2023

I'd like to suggest something in between:

let sillyfuncWithParams 
        parameterName1
        ignoredParameterName2 
        ignoredParameterName3 
        =
    longBody
    ...
    longBody

e.g. parameters idented and = with the same identation as parameters on the next line

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

9 participants