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

Allow multiple instances of the server on a single process. #51

Merged
merged 2 commits into from Jun 13, 2023

Conversation

TrevorThoele2
Copy link
Contributor

Addresses issue.

Allows singleton json formatter for serialize/deserialize.

startWithSetup creates json formatter each time.

@baronfel
Copy link
Contributor

Thanks for the issue, triage, and PR! Happy to take this and release a new version once the tests are green. It looks like CI is having some trouble with the new ones at the moment.

let setupEndpoints(_: LspClient): Map<string, System.Delegate> =
[] |> Map.ofList

let requestWithContentLength(request: string) =
Copy link

Choose a reason for hiding this comment

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

let requestWithContentLength(request: string) = $"Content-Length: {request.Length}\r\n\r\n{request}"

git can convert newlines in files, and since this is a string literal, it caused a problem when the underlying file bytes changed. It was coded on windows which is \r\n, but got converted to unix line endings on push which is \n only. The json rpc stream requires \r\n.

Their error message is actually bugged. They don't escape the slashes in their string so it prints an actual newline where they intended to print "\r\n". I'll probably make a PR over there.

Also note it can't be a @ literal string now, or that would literally put \r\n in the string instead of escaping and putting the return and new line bytes.

@@ -874,7 +874,8 @@ let private serializationTests =
Data = None }
testThereAndBackAgain item
]
Shotgun.tests ]
Shotgun.tests
StartWithSetup.tests ]

[<Tests>]
let tests = testList "LSP" [ serializationTests; Utils.tests ]
Copy link

Choose a reason for hiding this comment

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

I just notice with Utils.Tests being here, maybe StartWithSetup.tests should be too, rather than at the end of the serializationTests. Dunno what maintainers want.
let tests = testList "LSP" [ serializationTests; Utils.tests; StartWithSetup.tests ]

@TrevorThoele2
Copy link
Contributor Author

Apologies, I believe this PR should now pass the tests. Let me know if there's anything more that I need to do.

@baronfel
Copy link
Contributor

Awesome! Thanks for the contribution, we'll try and get a new version out quickly.

@baronfel baronfel merged commit a1aa291 into ionide:main Jun 13, 2023
3 checks passed
@baronfel
Copy link
Contributor

baronfel commented Jun 13, 2023

If you don't mind me asking @TrevorThoele2 / @zZeck - what are y'all using the library for? I'm always interested in knowing more about who the users are (aside from FSAC, naturally :) )

@TrevorThoele2
Copy link
Contributor Author

@baronfel We are working on creating a project we call the "state space analysis tool" for C#. We think it would be useful to see the state space of variables, functions and classes (any C# component, really) as it is declared, as the state space implicitly changes at call sites, and then aggregated state space across all usages of components. This is what we believe we are doing at some level whenever we simplify our code. Having a tool that can display this information automatically by hovering over a component in a file while working would allow quicker-to-access edits/refactorings while keeping the same overall topology/functionality of the component in question.

Essentially, calculable information displayed to the user in a VS Code extension by hovering is what we're after right now.

Here is the link to the GitHub repository if you are interested: https://github.com/TrevorThoele2/poke.

@baronfel
Copy link
Contributor

That sounds awesome, very interested in taking a look.

It looks like the release pipeline didn't kick off as expected, let me try to get that going again.

@baronfel
Copy link
Contributor

There we go, 0.4.15 is published on NuGet.org!

@TrevorThoele2
Copy link
Contributor Author

I have pulled that version into our project and the tests are passing in the ways I expect them to. Thank you for your help, we really appreciate it! This is a very good and useful project for us and we appreciate that as well.

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