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 FSharp.Editor dependency on FSharp.Language Service redux #4132

Merged
merged 8 commits into from
Feb 22, 2018
Merged

Remove FSharp.Editor dependency on FSharp.Language Service redux #4132

merged 8 commits into from
Feb 22, 2018

Conversation

cartermp
Copy link
Contributor

@cartermp cartermp commented Dec 17, 2017

Rebase with reverted commits sucks.

This PR:

  • Moves ProjectSitesAndFiles into FSharp.Editor
  • Moves the remaining helper code from FSharp.LanguageService into FSharp.Editor
  • Removes the dependency on FSharp.LanguageService from FSharp.Editor
  • Adjusts calls in the legacy project system to call the correct ProjectSitesAndFiles code
  • Adjusts legacy language service tests which only test deprecated functionality to no longer depend on FSharp.Editor
  • Adjusts legacy project system tests to call the correct things
  • Does minor cleanup

Tested:

  • Creation of .NET Framework and .NET Core SDK-based projects
  • Mixing of .NET Framework and .NET Standard projects, with references
  • File ordering in the above
  • New files in the above
  • Script files in the above
  • IDE features work in the above

There is still a possible need for an integration test which verifies that when the old project system loads a project, the language service is successfully initialized. This apparently doesn't exist in our test suite, hence #4108 ultimately failing due to lacking the correct changes to calls in the legacy project system.

@cartermp cartermp changed the title [WIP] Remove FSharp.Editor dependency on FSharp.Language Service redux Remove FSharp.Editor dependency on FSharp.Language Service redux Jan 6, 2018
@cartermp
Copy link
Contributor Author

cartermp commented Jan 6, 2018

This is no longer WIP. I installed this PR into my F# tools and used it for a few days, and noticed that #4137 was occurring, but after @dwed confirmed that he also saw that long before this PR was ever created, I don't think that assertion is related.

This is not critical for 15.6, but I think it's definitely worth getting in for 15.7.

@dsyme
Copy link
Contributor

dsyme commented Jan 12, 2018

@cartermp I seem to remember an earlier version of this causing problems - was that the scare around #4137 or did the first version really have an issue? thanks

@cartermp
Copy link
Contributor Author

The first version did have an issue where the language service wasn't being initialized in .NET Framework projects (which CI didn't catch, heh). I fixed it here.

@dsyme
Copy link
Contributor

dsyme commented Jan 12, 2018

@cartermp OK, great :)

@cartermp cartermp added this to the 15.7 milestone Jan 20, 2018
@KevinRansom
Copy link
Member

@dotnet-bot test this please.

@KevinRansom KevinRansom merged commit e1e4cce into dotnet:master Feb 22, 2018
@dsyme
Copy link
Contributor

dsyme commented Feb 22, 2018

@cartermp After this PR the FSharp.ProjectSystem (old project system) still contains this reference:

    <ProjectReference Include="..\FSharp.LanguageService\FSharp.LanguageService.fsproj">
      <Name>FSharp.LanguageService</Name>
      <Project>{ee85aab7-cda0-4c4e-bda0-a64ccc413e3f}</Project>
      <Private>True</Private>
    </ProjectReference>

Is this intended, e.g. is there going to be multiple copies of state in FSharp.LanguageService.dll and FSharp.Editor.dll?

@cartermp
Copy link
Contributor Author

@dsyme I could take a look at pulling out that ref today. I intended only to cut ties with FSharp.LanguageService from FSharp.Editor, but if we can completely orphan it save for unit tests then that's preferrable

@dsyme
Copy link
Contributor

dsyme commented Feb 22, 2018

if we can completely orphan it save for unit tests then that's preferrable

Definitely. And if we can't I fear a bug :)

@cartermp cartermp deleted the move-projectsitesandfiles branch February 22, 2018 18:32
@cartermp
Copy link
Contributor Author

cartermp commented Feb 22, 2018

So removing that reference is fine, but I did notice that #4376 shows up if I build a VSIX of the latest master into 15.6 preview 5. I haven't done itfor some time, so I have no idea if it's this or not.

@cartermp
Copy link
Contributor Author

Confirmed that this PR does not cause #4376

nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
…net#4132)

* Move ProjectSitesAndFiles to FSharp.Editor

* Remove weird stuff and add the VSLang proj reference like it is in FSharp.LanguageService

* Remove dependency on FSharp.LanguageService

* Whoopsie on that legacy project system

* Update project system tests

* Update tests

* Weird comment removal

* Remove FSharp.Editor dependency on FSharp.Language service and update all other pieces
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