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 flycheck update the workspace that changed #11038

Closed
wants to merge 1 commit into from

Conversation

pitaj
Copy link
Contributor

@pitaj pitaj commented Dec 17, 2021

(Maybe) Fixes #8631

A couple problems:

  1. Seems like check diags from the previous loop are lost for workspaces that aren't flycheck updated. (Not sure why)
  2. Crates in skipped editor-workspaces that depend on the crate that was modified will not be rechecked. This only hurts if pub items were modified, which should be pretty rare if working on them independently. Can always use a cargo-workspace to avoid this.

@Veykril
Copy link
Member

Veykril commented Apr 3, 2022

Apologies for having let this PR rot for a few months.

Having read the linked issue I don't think this is the proper way forward to fix it mainly because of

Crates in skipped editor-workspaces that depend on the crate that was modified will not be rechecked. This only hurts if pub items were modified, which should be pretty rare if working on them independently. Can always use a cargo-workspace to avoid this.

This seems like a terrible user experience, if I change a crate I want to see the impact on all its dependants.

So I am inclined to close this PR, we should instead investigate on reducing the flychecks to the crate that changed and its reverse dependencies.

@Veykril Veykril closed this Apr 3, 2022
@pitaj
Copy link
Contributor Author

pitaj commented Apr 3, 2022

Obviously this isn't a full working solution. I pointed out those problems because I was hoping to get feedback on how to go about fixing them.

@Veykril
Copy link
Member

Veykril commented Apr 3, 2022

Ah I apologize then, I unfortunately don't know enough about that part of the codebase currently. I'll have to see if I can find the time to get myself accustomed with that, though that might take a bit.

@Veykril
Copy link
Member

Veykril commented Jul 18, 2022

So I looked into this, and my initial plan was to dig and then tell you about my findings but as it turned out this became more convoluted than I anticipated so I opened a new PR regarding this which will supercede this PR. Sorry, I hope you don't take this the wrong way :) Though the problem with your second point still persists, haven't looked into that yet #12808

@Veykril Veykril closed this Jul 18, 2022
};
if abs_path.starts_with(root) {
flycheck.update();
// FIXME: break here? the file should only belong to one workspace
Copy link
Member

Choose a reason for hiding this comment

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

#[path] and include! would beg to differ 🥲.

bors added a commit that referenced this pull request Aug 4, 2022
feat: Only flycheck workspace that belongs to saved file

Supercedes #11038

There is still the problem that all the diagnostics are cleared, only clearing diagnostics of the relevant workspace isn't easily doable though I think, will have to dig into that
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.

rust-analyzer tries to check all crates in vscode workspace in parallel
3 participants