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

Feature: Automatically restart language client when config or rules files change #75

Merged
merged 5 commits into from
Feb 4, 2024

Conversation

StevenLove
Copy link
Member

Especially for new users, configuring ast-grep in VSCode can be difficult because you can forget to reload the extension in which case your changes to sgconfig.yml or to your rule files will have no effect on the reported diagnostics and code actions. This PR uses vscode.workspace.FileSystemWatchers to monitor sgconfig.yml and the rules files found at ruleDirs[0] and restarts the LanguageClient when these files change. This PR includes mocha tests which verify the desired behavior and which add about 1s to npm run test

package.json Show resolved Hide resolved
scripts/test/integration-test.cjs Show resolved Hide resolved
scripts/test/testSetup.ts Show resolved Hide resolved
src/extension.ts Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Show resolved Hide resolved
…tartLanguageClient' to convey that it does not immediately restart. 2) unsorted package.json dependencies which were automatically sorted by npm install. 3) removed a commented console.log statement. 4) Undid accidental upgrade of prettier from 3.0.0 to 3.4.0
Copy link
Member

@HerringtonDarkholme HerringtonDarkholme left a comment

Choose a reason for hiding this comment

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

Thanks!

@HerringtonDarkholme
Copy link
Member

Looks like the two tests failed on Windows. Would you like to give it a look?

     1) Update on new rule creation
    2) Update on ruleDirs change back to real path

…for initial diagnostics to match their expectations, before actually testing diagnostic changes from file modifications
@HerringtonDarkholme
Copy link
Member

thanks! I'm running the test again and hope it pass!

@HerringtonDarkholme
Copy link
Member

I'm merging this anyway since the windows fix is not okay anyway. If possible, would you like to have a separate pr to fix the issue? thanks!

@HerringtonDarkholme HerringtonDarkholme merged commit 8d7ade2 into ast-grep:main Feb 4, 2024
2 of 3 checks passed
@StevenLove
Copy link
Member Author

Thank you for your attention to detail in reviewing my code and for your helpful comments. I know you are busy so I've tried to avoid spamming you with notifications as best I know how.
I'm starting to accept that it may take some work to get the tests to pass on the windows CI, and I don't have a proper setup to do local windows development at the moment. I appreciate your decision to accept the PR and allow another PR to fix the test for windows.
There are some additional changes I would like to implement (the remaining TODOs) and perhaps I will gain some understanding of what is happening with windows as I make progress on those other changes.

I would appreciate any advice about how I can improve my contribution. Thank you.

@HerringtonDarkholme
Copy link
Member

Hi @StevenLove, the failing windows test has been fixed by pinning dep versions.

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