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

Fix Tests for AdaptiveLspServer #1053

Merged
merged 16 commits into from Feb 19, 2023
Merged

Conversation

TheAngryByrd
Copy link
Member

@TheAngryByrd TheAngryByrd commented Feb 4, 2023

Notable changes:

  • Fixes remaining tests for Adaptive LSP Server.
  • Fixes ReferenceAssemblies interfering with type checks across projects on net7.0 project
  • Adds ability to timeout individual tests to prevent whole test suite from stalling

Copy link
Member Author

@TheAngryByrd TheAngryByrd left a comment

Choose a reason for hiding this comment

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

Some initial feedback before I forget it.

src/FsAutoComplete.Core/Commands.fs Show resolved Hide resolved
src/FsAutoComplete/LspServers/AdaptiveFSharpLspServer.fs Outdated Show resolved Hide resolved
@TheAngryByrd TheAngryByrd marked this pull request as ready for review February 8, 2023 04:56
@Krzysztof-Cieslak
Copy link
Member

That looks really amazing :-)

Comment on lines +464 to +478
for file in System.IO.Directory.EnumerateFiles(path, "*.fsproj", SearchOption.AllDirectories) do
do! file |> Path.GetDirectoryName |> dotnetRestore
Copy link
Member Author

Choose a reason for hiding this comment

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

We weren't doing restore properly for anything that contained subfolders with fsprojs

Comment on lines +118 to +122
fun toolsPath ->
if projectGraphEnabled then
Ionide.ProjInfo.WorkspaceLoaderViaProjectGraph.Create(toolsPath, ProjectLoader.globalProperties)
else
Ionide.ProjInfo.WorkspaceLoader.Create(toolsPath, ProjectLoader.globalProperties)
Copy link
Contributor

Choose a reason for hiding this comment

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

You mentioned elsewhere about passing in 'other CLI properties' to proj-info - what were you thinking about?

Copy link
Member Author

Choose a reason for hiding this comment

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

Wonder if it's worth having a --msbuild-global-properties CLI flag for FSAC that you could do like MSBuildProperty=Foobar,AnotherProperty=false and pass to proj-info it's not a hardcoded list. More escape hatch kind of stuff if msbuild related stuff changes we don't have to release a fix right away if an sdk change or whatever breaks FSAC.

Copy link
Contributor

Choose a reason for hiding this comment

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

ohhhhh, I see. That would be a nice addition, but to me it almost speaks to a problem in proj-info instead - we need to be able to change the global properties during runtime and re-trigger evaluation based on that. Classic example is how we hard-code Debug right now. So instead of a CLI flag I think I'd prefer a config setting in Ionide that would map to a JSON-RPC call to set specific global properties for a project, which would flow into proj-info as required. This is a bit bigger of a problem space, though.

Copy link
Contributor

@baronfel baronfel left a comment

Choose a reason for hiding this comment

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

Great work :) happy to merge this, it's a huge confidence-boost for the adaptive server!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants