Skip to content

Conversation

@dsyme
Copy link
Contributor

@dsyme dsyme commented Feb 19, 2019

This is a spike of a cleanup to split service.fs into

  • FSharpCheckerResults.fs/fsi
  • FSharpChecker.fs/fsi

along with a little rejigging to make the abstraction boundary cleaner (in particular TypeCheckInfo is completely internal to the former)

There's about 2000 LOC in the former and 1200 LOC in the latter.

This work would conflict with changes in dev16.0 and I think should probably just go into that branch (I guess I'd do the work to do the merge, or redo the work completely)

@dsyme dsyme closed this Feb 19, 2019
@dsyme dsyme reopened this Feb 19, 2019
@dsyme
Copy link
Contributor Author

dsyme commented Feb 19, 2019

@cartermp Same test failed:

at FSharp.Compiler.Service.Tests.PerfTests.Test request for parse and check doesn't check whole project() in D:\a\1\s\tests\service\PerfTests.fs:line 75

@dsyme
Copy link
Contributor Author

dsyme commented Feb 19, 2019

Two remaining test failures, I can't tell if it's transient - both TypeProvider disposal smoke tests failed. I'll re-run tests.

@dsyme dsyme closed this Feb 19, 2019
@dsyme dsyme reopened this Feb 19, 2019
@dsyme
Copy link
Contributor Author

dsyme commented Feb 19, 2019

After discussing with @TIHan and @cartermp we will wait until dev16.0 integrates into master before merging this

@dsyme dsyme closed this Apr 2, 2019
@dsyme dsyme reopened this Apr 2, 2019
@cartermp
Copy link
Contributor

@dsyme is this still WIP?

@cartermp
Copy link
Contributor

Also it seems like a good opportunity to normalize more whitespace here (e.g., line breaks)

@dsyme dsyme changed the title [WIP] split service.fs split service.fs Apr 14, 2019
@dsyme
Copy link
Contributor Author

dsyme commented Apr 14, 2019

Also it seems like a good opportunity to normalize more whitespace here (e.g., line breaks)

I'll do in a separate PR - this PR is splitting a large file in half which is a bit annoying as git loses the source history tracking with the half of the file moved into the new file.

In case we ever needed it I created couple of branches that first duplicate "service.fs" and "service.fsi" into "FSharpCheckerResults.fs" and "FSharpCheckerResults.fsi" and then applies this diff, dsyme/fsharp@tmp2...clean5b

That's a more realistic view of the changes in this PR

@dsyme dsyme closed this Apr 16, 2019
@dsyme dsyme reopened this Apr 16, 2019
@dsyme
Copy link
Contributor Author

dsyme commented Apr 16, 2019

@TIHan This is ready. Could you review?

This may be better for review: dsyme/fsharp@tmp2...clean5b

@dsyme
Copy link
Contributor Author

dsyme commented Apr 24, 2019

Unexpected unrelated failure in Match01.fs baseline test, runnng #6629 to check if this is a failure in master

@dsyme dsyme closed this May 22, 2019
@dsyme dsyme reopened this May 22, 2019
@dsyme
Copy link
Contributor Author

dsyme commented May 22, 2019

Looks like the failure from "visualfsharp-CI — #20190424.3 failed" is fake, a left over from our old build web hook, now replaced. Not sure how to clear that short of opening a new PR.

@dsyme
Copy link
Contributor Author

dsyme commented Jun 5, 2019

@TIHan Can you approve? I'd like to get this in (it is a PITA to maintain :) )

@dsyme
Copy link
Contributor Author

dsyme commented Jun 5, 2019

@TIHan I'd like to keep working on pulling apart some of the monolithic files, I think it will help

@@ -0,0 +1,662 @@
<?xml version="1.0" encoding="utf-8"?>
Copy link
Contributor

@TIHan TIHan Jun 5, 2019

Choose a reason for hiding this comment

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

This (fsproj file) shouldn't be here as buildfromsource is gone from master.

Copy link
Contributor

@TIHan TIHan left a comment

Choose a reason for hiding this comment

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

This looks fine to me. We simply just refactored out the checker results into a separate file.

One comment on the fsproj in buildfromsource. Basically that fsproj should be removed.

@TIHan TIHan merged commit f72b7b2 into dotnet:master Jun 6, 2019
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