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

LSP: Preparations and Cleanup #687

Merged
merged 11 commits into from
Dec 29, 2023
Merged

LSP: Preparations and Cleanup #687

merged 11 commits into from
Dec 29, 2023

Conversation

falko17
Copy link
Collaborator

@falko17 falko17 commented Dec 23, 2023

Warning

Please do not delete the lsp branch yet, as I will keep working on it.

Summary

Just in time for Christmas, this prepares the code base for the upcoming integration of the Language Server Protocol into SEE (#686). Additionally, this should resolve license timeouts in the CI, which have recently quite often caused the pipeline to fail.

NuGetForUnity

This integrates NuGetForUnity into SEE, which makes it a lot easier to integrate NuGet packages into SEE without having to extract the package, update it, pull in dependencies, etc.
This additionally has the nice property of downsizing our repository, since only the package names are stored in the repository and the packages themselves are resolved locally.
I have removed all existing packages from the repository that we got from NuGet in the past (FuzzySharp, HtmlAgilityPack, XZCompression) and tracked them in NuGetForUnity.

Note

In the future, when we want to import NuGet packages into SEE, we should do it using the NuGet menu option at the top of Unity.

Other changes

  • In preparation of Implement support for the Language Server Protocol #686, I have added the OmniSharp LSP Client and associated LSP data models.
    • Apparently, this also caused Microsoft's C# Language Analyzer to activate, which caused a few additional diagnostics and warnings to be shown. I fixed these warnings, which were mainly about async methods' names having to end in Async (such that they are not easily overlooked, as they have to be awaited). These fixes account for the majority of changes in this pull request.
  • I have cleaned up the CodeWindow (and related classes) and removed all multiplayer and editing capabilities, such as the CRDT. The reason is that 1) it currently does not work and makes the game freeze up, 2) it causes the code to be vastly more complex and less maintainable, and 3) when it did work, it worked neither reliably nor with good performance (due in part to our implementation and in part due to TextMeshPro complexities). If we want this capability back in the future, we should probably rewrite it.
  • The CI scripts have been updated to hopefully cause less trouble when running the tests and creating the builds. Previously, they would often time out, causing many false positives – this should now be fixed.

This also pulled in quite a bit of other dependencies, including
Microsoft's C# Language Analyzer. Hence, some additional warnings
and diagnostics appeared.
@falko17 falko17 added enhancement New feature or request no release No new release will be created after a merge when setting this on a PR. labels Dec 23, 2023
@falko17 falko17 requested a review from koschke December 23, 2023 22:38
@falko17 falko17 self-assigned this Dec 23, 2023
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

There are a few bad patterns I found which you should check.

Assets/SEE/UI/ConfigMenu/Switch.cs Outdated Show resolved Hide resolved
@koschke
Copy link
Collaborator

koschke commented Dec 24, 2023

@falko17 Well timed indeed.

@falko17
Copy link
Collaborator Author

falko17 commented Dec 24, 2023

Unfortunately, the new build script versions still cause some trouble with permissions I have to fix before this can be merged.

@falko17
Copy link
Collaborator Author

falko17 commented Dec 24, 2023

Unfortunately, the new build script versions still cause some trouble with permissions I have to fix before this can be merged.

Fixed it just in time for actual Christmas Day. This is now ready to review and merge. As it turned out, the problem was a bug in our Unity CI runner. I submitted a pull request that fixes the bug, and switched our pipeline to use my local fork for now.

@msteinbeck
Copy link
Collaborator

In preparation of #686, I have added the OmniSharp LSP Client and associated LSP data models.

@m4xxed has some experience with this LSP client.

@falko17
Copy link
Collaborator Author

falko17 commented Dec 25, 2023

@m4xxed has some experience with this LSP client.

Ah, good to know, thanks!

Copy link
Collaborator

@koschke koschke left a comment

Choose a reason for hiding this comment

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

Looks good to me. Only one clarification question and one documentation request.

.github/workflows/main.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

@koschke koschke left a comment

Choose a reason for hiding this comment

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

All things addressed. Good to go.

@koschke koschke enabled auto-merge December 29, 2023 14:44
@koschke koschke merged commit 8e654bb into master Dec 29, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request no release No new release will be created after a merge when setting this on a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants