-
Notifications
You must be signed in to change notification settings - Fork 831
Remove dependency on FSharp.LanguageService #4108
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea what this is, but it was added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think any of this is necessary. After commenting this out, it builds, and the VSIX that I installed into my VS appears to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no clue why the full version and stuff was specified, but this was just through right-click > add reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I figured out how to deal with this
|
Assuming this passes CI, I think this is ready. I'm still manually testing it, but the following seems to work just like the current tools:
So I don't think I missed anything. Still, it would be nice to have at least another person try this out. |
|
All of the above testing also works for .NET Core SDK projects. |
|
Is the purpose of this to eliminate the Language Service dll? If yes, how much work is needed to complete that? or can we just delete it all now? |
|
This is a step in that direction, yes. I'm currently looking to do that right now, and there are a few more things which would need moving over. The File System completion logic in LanguageService.Base (ported from Roslyn) is currently blocking removal of dependencies on LanguageService.dll and LanguageService.Base.dll. |
|
When do you plan on completing it? Christmas, VS 15.6, 2020, or the heat death of the Sun :-) ? Kevin |
|
Probably by christmas, but I made this PR as-is so that it can be merged if I can't get it decoupled in time. And yes, FSharp.Editor is the right location for now. Once we are no longer referencing these projects which are basically just dead code paths, we can start to migrate tests over to no longer use those code paths. Then, finally, we can delete the dead code. It's a long-term effort that I want to personally start addressing. This is one of the steps in that process. |
|
And as it turns out, there was just one tiny thing to move over from the FSharp.LanguageService project. Dependency on it is now gone! |
|
This is ready, but as always, I welcome any additional testing. |
KevinRansom
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
| <Name>FSharp.Core</Name> | ||
| </ProjectReference> | ||
| <ProjectReference Include="$(FSharpSourcesRoot)\..\vsintegration\src\FSharp.LanguageService\FSharp.LanguageService.fsproj"> | ||
| <!-- <ProjectReference Include="$(FSharpSourcesRoot)\..\vsintegration\src\FSharp.LanguageService\FSharp.LanguageService.fsproj"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just delete this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops, forgot that.
|
@cartermp This completely breaks legacy projects from having any tooling. What is your plan for fixing that bit? |
|
Alright I've gaslit myself:
And yet I spent at least 30 minutes on a built VSIX of this PR verifying that nothing was broken for both project flavors. Time to revert this and wonder what the hell is going on in this untested code |
* Move ProjectSitesAndFiles to FSharp.Editor * Remove commented out code * Fix build that wasn't reported when running build.cmd * Remove weird stuff and add the VSLang proj reference like it is in FSharp.LanguageService * Remove dependency on FSharp.LanguageService * remove commented out reference
…otnet#4128) This reverts commit 351d06b.
It was bothering me that there were live code paths in the FSharp.LanguageService project necessary for .NET Core SDK-based project support. I moved those code paths into the FSharp.Editor project, keeping the old one intact and modifying a few things to disambiguate types. We still have a bundle of spaghetti for project references, but at least this is one less set of code paths in FSharp.Language which won't be executed on peoples' machines.