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

Only one server restart at a time #194

Merged
merged 2 commits into from
May 23, 2023
Merged

Conversation

konstin
Copy link
Member

@konstin konstin commented May 16, 2023

This hopefully solves #127

I still have to find a better way to test this

src/extension.ts Outdated
@@ -33,7 +35,28 @@ export async function activate(context: vscode.ExtensionContext): Promise<void>
traceVerbose(`Configuration: ${JSON.stringify(serverInfo)}`);

const runServer = async () => {
client = await restartServer(serverId, serverName, outputChannel, client);
if (clientPromise) {
Copy link
Member

Choose a reason for hiding this comment

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

clientPromise != null

Let's not depend on implicit conversion (it's safe here because it is a promise). We should add eslint or Rome

Copy link
Member Author

@konstin konstin May 17, 2023

Choose a reason for hiding this comment

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

Please add the appropriate tooling! I'm not up to date what the appropriate tooling in the js ecosystem is.

Copy link
Member Author

Choose a reason for hiding this comment

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

i tried adding eslint and it didn't flag this, i think i got the wrong set of rules

Copy link
Member

Choose a reason for hiding this comment

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

Oh sorry. I missed this comment. We can add eslint (or Rome :P) as a separate PR.

@konstin
Copy link
Member Author

konstin commented May 17, 2023

I confirmed that the code works but doesn't fix the problem. Not sure what to do with this

@konstin konstin marked this pull request as ready for review May 17, 2023 15:49
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

I would merge it because it fixes one potential cause for the race and may provide us with additional information to fix it properly.

@konstin konstin merged commit 20215be into main May 23, 2023
@konstin konstin deleted the one_server_restart_at_a_time branch May 23, 2023 10:43
charliermarsh added a commit that referenced this pull request Jun 16, 2023
## Summary

In #194, we stopped setting the `client` returned by the
`clientPromise`. As a result, the "Fix all" action stopped working,
since it relies on the `client`.

Closes #194.
charliermarsh added a commit that referenced this pull request Jun 17, 2023
## Summary

This PR overhauls the LSP client to pull in a bunch of improvements
that've been made to the upstream template, primarily through these two
PRs:

-
microsoft/vscode-python-tools-extension-template#100
-
microsoft/vscode-python-tools-extension-template#99

Some of the issues that those PRs fixed, we've actually fixed already --
but that team knows the LSP and VS Code much, much better, and so I'm
happy to pull in their changes.

We've obviously diverged a bit from the original template, so pulling in
these changes while preserving our own deviations is tricky... But, in
particular, I had to do the following:

- Change the code in `settings.ts` to match our settings schema,
defaults, etc.
- Change the code in `extension.ts` to register our custom actions.
- Change the code in `extension.ts` to support enabling and disabling
the extension (#222).
- Change the code in `extension.ts` to debounce restarts (#194, #218).
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.

2 participants