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

Remove IVTs to legacy language service #7001

Merged
merged 5 commits into from
Jun 19, 2019
Merged

Remove IVTs to legacy language service #7001

merged 5 commits into from
Jun 19, 2019

Conversation

cartermp
Copy link
Contributor

Moved the dead code paths to no longer be IVT into FSharp.Compiler.Private.

@cartermp
Copy link
Contributor Author

I have no clue what the ever loving hell this test does and why but it's failing:

member public this.``PublicSurfaceArea.DotNetReflection``() =
let ps = publicTypesInAsm @"FSharp.ProjectSystem.FSharp.dll"
Assert.AreEqual(1, ps) // BuildPropertyDescriptor
let ls = publicTypesInAsm @"FSharp.LanguageService.dll"
Assert.AreEqual(0, ls)
let compis = publicTypesInAsm @"FSharp.Compiler.Interactive.Settings.dll"
Assert.AreEqual(5, compis)
let compserver = publicTypesInAsm @"FSharp.Compiler.Server.Shared.dll"
Assert.AreEqual(0, compserver)
let lsbase = publicTypesInAsm @"FSharp.LanguageService.Base.dll"
Assert.AreEqual(0, lsbase)
let psbase = publicTypesInAsm @"FSharp.ProjectSystem.Base.dll"
Assert.AreEqual(17, psbase)
let fsi = publicTypesInAsm @"FSharp.VS.FSI.dll"
Assert.AreEqual(1, fsi)

@cartermp
Copy link
Contributor Author

I modified the code to use the public API for the information it needs and removed the test in question. I can't for the life of me understand the value in that test.

@KevinRansom
Copy link
Member

@cartermp Well, the tests are a sort of cheap evaluation of whether public types have been added or removed from those assemblies. If public types are removed its potentially a breaking change, if they are added it's potentially an unintentional increase in surface are.

But that test does the job in a pretty bad way. And probably adds little value.

@cartermp
Copy link
Contributor Author

Yeah, in this case I don't think they're of value here because the primary things they're testing are now dead code paths anyways

@KevinRansom KevinRansom merged commit b900968 into dotnet:master Jun 19, 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.

2 participants