flycheck: initial implementation of $saved_file
#15381
Conversation
rustbot
added
the
S-waiting-on-review
davidbarsky
force-pushed
the
davidbarsky/implement-saved-file
branch
from
6c36c2c to
397fed5
Compare
There was a problem hiding this comment.
I'll have to check up with my old related PR (that you've linked) to see if this works out together later,
Implements the "potential solution" in outlined in #12882.
I don't see where this is related. The PR here merely implements the logic for helping out non cargo based build tools.
| match self { | ||
| FlycheckConfig::CustomCommand { invoke_with_saved_file, .. } => *invoke_with_saved_file, | ||
| _ => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
| match self { | |
| FlycheckConfig::CustomCommand { invoke_with_saved_file, .. } => *invoke_with_saved_file, | |
| _ => false, | |
| } | |
| } | |
| matches!(self, FlycheckConfig::CustomCommand { invoke_with_saved_file: true, .. }) | |
| } |
| @@ -163,6 +174,7 @@ struct FlycheckActor { | |||
| /// Either the workspace root of the workspace we are flychecking, | |||
| /// or the project root of the project. | |||
| root: AbsPathBuf, | |||
| state: OnceCell<FlycheckState>, | |||
There was a problem hiding this comment.
What's the reason for having this?
There was a problem hiding this comment.
It solves a cancellation-related bug: a flycheck would be started, canceled before completion, and restarted. The restarted flycheck would then not interpolate the $saved_file value.
I think with some deeper changes to Flycheck, this can be replaced by a last_saved_file, but this introduction of statefulness one reason I have some misgivings about this approach.
Veykril
added
S-waiting-on-author
I get some not-great vibes from this implementation. It might work, but there's a messiness here that's bothering me.
I believe I was referring to the |
|
Ah you meant to link #13437 then I think |
I'll have to give this topic another look in 2 weeks then probably (kind of exhausted most of my general r-a budget this week and then I'm on vacation) . |
No worries, enjoy your vacation! I'm going to think about this approach during this time. |
|
☔ The latest upstream changes (presumably #15465) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I'm going to close this in favor of #15476 (Wilfred and I work together). |
Substitute $saved_file in custom check commands If the custom command has a $saved_file placeholder, and we know the file being saved, replace the placeholder and run a check command. If there's a placeholder and we don't know the saved file, do nothing. This is a simplified version of #15381, which I hope is easier to review.
Substitute $saved_file in custom check commands If the custom command has a $saved_file placeholder, and we know the file being saved, replace the placeholder and run a check command. If there's a placeholder and we don't know the saved file, do nothing. This is a simplified version of #15381, which I hope is easier to review.
I don't think this is the cleanest or best code I've ever written, but paired with the changes in facebook/buck2@8410bca0dda8a499065c4651774c33b7143c4983—which I've been using personally for the last week or so—this solution works pretty nicely. This PR takes inspiration from #13528 and implements the "potential solution" in outlined in #12882.