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

Implement did_close() and per-file diagnostics #81

Merged
merged 5 commits into from
Aug 30, 2023

Conversation

DavisVaughan
Copy link
Contributor

@DavisVaughan DavisVaughan commented Aug 28, 2023

A step towards posit-dev/positron#1005

Filled in did_close(), which should be useful if we decide we want to regenerate diagnostics for all open files.

Also switched away from using a global variable for DIAGNOSTICS_VERSION altogether. I believe we can safely use the per-file version for this, which is thread safe due to it being inside a DashMap (aka mimicking a RwLock<HashMap>). This change allows multiple files to enqueue diagnostics at the same time (i.e. we no longer cancel file A's diagnostics if file B enqueue's theirs), which will also be necessary for regenerating diagnostics for all open files.

I originally thought I was having some kind of deadlock issue with this switch, but I think I may have just compiled a faulty version of ark on my end because the problem went away after a full recompile. It does make me slightly nervous though. I will keep an eye out for anything suspicious over the next few days.


454bae0 contains the switch to using a per-file version check, a941b77 is some further tweaking of enqueue_diagnostics_impl() so that it no longer needs to be async and has this clear responsibility of Document in -> Diagnostics out

Copy link
Contributor

@kevinushey kevinushey left a comment

Choose a reason for hiding this comment

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

LGTM!

@DavisVaughan DavisVaughan merged commit 47ed756 into main Aug 30, 2023
@DavisVaughan DavisVaughan deleted the feature/diagnostic-improvements branch August 30, 2023 18:10
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