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

Print in keyword after correct SynBinding #1180

Merged
merged 1 commit into from
Oct 2, 2020
Merged

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Oct 2, 2020

Fixes #1176

@nojaf nojaf merged commit a241023 into fsprojects:master Oct 2, 2020
@nojaf nojaf deleted the fix-1143 branch October 2, 2020 09:38
@@ -2000,7 +2000,7 @@ and genExpr astContext synExpr =
mkRange "IN" binding.RangeOfBindingAndRhs.End e.Range.Start

Map.tryFindOrEmptyList IN ctx.TriviaTokenNodes
|> TriviaHelpers.``keyword token inside range`` inRange
|> TriviaHelpers.``keyword token after start column and on same line`` inRange
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably a better thing to do here would be looking inside the range between the last binding end and the body expression start.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, thought of that but we capture nested LetOrUse expressions so that information is lost in CodePrinter.
See

let rec (|LetOrUses|_|) =

trivia
|> List.choose (fun t ->
match t.Type with
| TriviaNodeType.Token (_, tok) when (range.StartLine = t.Range.StartLine
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it need to handle cases where in is on another line?

do
    let _ = ()
      in
     () // note the different indent is allowed here due to `in` use

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I think it should since we are doing this.
Thanks for pointing that out.

@nojaf nojaf mentioned this pull request Oct 2, 2020
3 tasks
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.

Only add one in keyword in LetOrUse
2 participants