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

Initial chain proof of concept. #2696

Merged
merged 7 commits into from
Jan 12, 2023
Merged

Initial chain proof of concept. #2696

merged 7 commits into from
Jan 12, 2023

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Jan 9, 2023

An attempt to address fsharp/fslang-design#688.
This is merely a suggestion on how to provide guidance. Nothing has been decided so far.

The gist of this PR is that everything that is SynExpr.DotGet related is being converted to an Expr.Chain. A ChainNode consists of links. Each link is either a dot or something between a dot.

Examples:

A.B.C().D
E.F(fun g -> g).H
I<j>().K[0].L(m,n,o).P

The expression between a dot could be multiple things:

  • An identifier such as A or A<b>
  • An application with unit A()
  • An application with a parenthesis expression A(b) (including lambdas A(fun b -> b))
  • An index expression .[A]

Capturing all these different F# AST shapes into one unified model is already a big win.
A lot of very selective AST combinations can now easily be replaced with this. Regardless of the what the style will end up being.

Copy link
Member

@dawedawe dawedawe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it

src/Fantomas.Core.Tests/ChainTests.fs Outdated Show resolved Hide resolved
@@ -1245,7 +1190,8 @@ m
.Property(fun p -> p.Name)
.IsRequired()
.HasColumnName("ModelName")
.HasMaxLength 64
.HasMaxLength
64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nojaf shouldn't this single line above have one extra level of indentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think so, 64 is the argument of the DotGet application.
Syntactically, this is the equivalent of

let f =
    m
        .Property(fun p -> p.Name)
        .IsRequired()
        .HasColumnName("ModelName")
        .HasMaxLength

f 64

If the application doesn't fit on the rest of the line, the arguments are applied indented on the following line.

f
   64

Another extreme example of this would be:

(try
    id
 with ex ->
     id)
    64

In practice, I would write the code with parentheses

    m
        .Property(fun p -> p.Name)
        .IsRequired()
        .HasColumnName("ModelName")
        .HasMaxLength(64)

Copy link
Contributor

@knocte knocte Jan 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, 64 looks more like the argument that is passed to the last func, and as such, would visually be better place there:

    .Property(fun p -> p.Name)
    .IsRequired()
    .HasColumnName("ModelName")
    .HasMaxLength
        64

But this is just because that 64 is the last one and is not another fluent call (it doesn't start with a dot). If it started with a dot, then I'd agree with you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m
    .Property(fun p -> p.Name)
    .IsRequired()
    .HasColumnName("ModelName")
    .HasMaxLength 64
    .HasMaxLength
        64

This is no longer part of the same chain that starts with m.
What you wrote there is:

(m.Property(fun p -> p.Name).IsRequired().HasColumnName("ModelName").HasMaxLength)
     (64.HasMaxLength 64)

You can only add that space at the very end, the same reason "ModelName" is not applied with a space but has parentheses.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't mean to write HasMaxLength twice, that was a typo, I edited my message 5 seconds after posting it

@nojaf nojaf merged commit b6f7ecf into fsprojects:main Jan 12, 2023
@nojaf nojaf deleted the poc/chain branch January 12, 2023 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants