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

Capture StartEndRange information in SynExpr.AnonRecd to fix #2566 #2591

Merged
merged 5 commits into from
Oct 24, 2022

Conversation

dawedawe
Copy link
Member

fixes the lost comments in #2566

Copy link
Contributor

@nojaf nojaf left a comment

Choose a reason for hiding this comment

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

A good start here! Please dot some more i's and cross some t's and we can merge this in.

src/Fantomas.Core/CodePrinter.fs Outdated Show resolved Hide resolved
src/Fantomas.Core/CodePrinter.fs Outdated Show resolved Hide resolved
src/Fantomas.Core/CodePrinter.fs Outdated Show resolved Hide resolved
src/Fantomas.Core/SourceParser.fs Outdated Show resolved Hide resolved
- pass openingBrace, closingBrace to genMultilineAnonRecord, too
- call genTriviaFor for openingBrace, too
- expand tests to cover opening braces
@dawedawe
Copy link
Member Author

Adding genTriviaFor for the opening braces shows a separator problem:
In copy expressions:

let a =
    {| foo with// test1

In record instances:

let x =
    {| Y =// test1

The later could be solved be using
genTriviaFor SynExpr_AnonRecd_OpeningBrace openingBrace sepOpenAnonRecdFixed
instead of
genTriviaFor SynExpr_AnonRecd_OpeningBrace openingBrace sepOpenAnonRecd
but I'm really not sure if this is the way to go here.

Copy link
Contributor

@nojaf nojaf left a comment

Choose a reason for hiding this comment

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

Looks good! Many thanks

@nojaf
Copy link
Contributor

nojaf commented Oct 21, 2022

You could force the comment to be printed after {|, similar to how it is done for normal records.

We can merge this PR as is, for my part. It's up to you.

@dawedawe
Copy link
Member Author

You could force the comment to be printed after {|, similar to how it is done for normal records.

Ah thanks, that was way easier than I thought.


let a =
{| // test1
foo with
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it is the same as we have it for regular records.
But in the future, we might want to have something like

{| // foo
   bar with
    A = 1
    B = 2 |}

I don't feel strongly about this, so no need to address this in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this should better be handled in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me!
I have no idea why the CI is failing, the NuGet lock file should not be throwing errors.
I think we need to apply the steps explained here.
I'll do that in a separate PR so you can rebase with main.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is also dotnet/fsharp#13678.
I hope it is ok.

@nojaf nojaf merged commit 529aa7d into fsprojects:main Oct 24, 2022
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.

2 participants