Skip to content

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?

let rec loop (input: range list) (res: range list list) currentBulk =
match input, currentBulk with
| [], [] -> 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