Skip to content

Merge consequent open decls and hash directives into single structure blocks#3971

Merged
KevinRansom merged 3 commits intodotnet:masterfrom
vasily-kirichenko:combine-sparse-open-decls-and-hash-directives
Nov 22, 2017
Merged

Merge consequent open decls and hash directives into single structure blocks#3971
KevinRansom merged 3 commits intodotnet:masterfrom
vasily-kirichenko:combine-sparse-open-decls-and-hash-directives

Conversation

@vasily-kirichenko
Copy link
Contributor

@vasily-kirichenko
Copy link
Contributor Author

@dotnet-bot test Ubuntu14.04 Release_fcs Build please

Copy link
Contributor

Choose a reason for hiding this comment

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

Are we better off with IsNullOrWhitespace here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

omg yes, I was sure I use this function :( Thanks for the review. Fixed.

@vasily-kirichenko vasily-kirichenko force-pushed the combine-sparse-open-decls-and-hash-directives branch from 9a6a79b to 01a235d Compare November 19, 2017 20:44
@dsyme
Copy link
Contributor

dsyme commented Nov 20, 2017

LGTM. Add a test to tests/service please

@vasily-kirichenko
Copy link
Contributor Author

@dsyme done.

@dsyme
Copy link
Contributor

dsyme commented Nov 21, 2017

@vasily-kirichenko Wow, thank you for such extensive tests, those are fantastic. Very impressive

| r :: rest, last :: _ when r.StartLine = last.EndLine + 1 ->
| r :: rest, last :: _
when r.StartLine = last.EndLine + 1 ||
sourceLines.[last.EndLine..r.StartLine - 2] |> Array.forall System.String.IsNullOrWhiteSpace ->
Copy link
Contributor

Choose a reason for hiding this comment

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

@vasily-kirichenko what is it that ensures that r.StartLine > 2 here? Is it possible that r.StartLine may = 1?

@@ -545,7 +545,9 @@ module Structure =
| [], [] -> List.rev res
| [], _ -> List.rev (currentBulk::res)
Copy link
Contributor

Choose a reason for hiding this comment

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

I imagine that currentBulk is a legacy typo for currentBlk, it probably makes sense to fix this while here.

@KevinRansom KevinRansom merged commit ee05187 into dotnet:master Nov 22, 2017
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.

4 participants