Closed ErwanDL closed 4 years ago
Thank you! This looks good.
I have no idea how ESLint might have gotten around the file modification problem. If you'd like to look into it, go ahead. I can't even begin to think of a solution, myself. For the time being, I'm happy with the warning.
Oh, yeah, the tidy module absolutely needs to be refactored. I wrote this plugin in just a few days, so it's bound to be a mess. I think a class with a nicer interface is probably the right way to go.
When you say "singleton", do you mean a class that can only be constructed once and that acts as a fancy global or just a class that we only bother to instantiate once? The latter is fine, but I don't see the need for the former.
Tests would absolutely be helpful. I don't even know where to begin with testing this project, honesty. I'd have to spend a couple hours to figure out a plan. I just can't find the time to work on this project right now, though. I haven't even been able to work on my hobby projects lately.
I mean "singleton" in the sense of the Singleton Pattern : a class that is only instantiated once, and returns the initial instance on subsequent calls. I think it makes sense as there should always be only one instance of clang-tidy running (it's really not hard to implement the singleton pattern in TS).
I can start working on figuring out what tests are needed and submit PRs that you can review when you have time.
For the file modification problem, I also think that it can wait for a bit.
As suggested on #19, I added a progress notification in the window when clang-tidy is linting/fixing the current file :
The message displayed is "Linting current file..." or "Linting and fixing current file (do not modify it in the meanwhile)...", depending on whether it should just lint or also fix the file.
Concerns/suggestions :
Since clang-tidy directly modifies the file on disk when fixing, you must not modify the file while it is being fixed by clang-tidy (if you do, the next time you save, you get an error message saying
The content of this file on disk is newer. Please compare or overwrite the changes
). I wrote that explicitly in the progress message, but this is clearly not enough. I think the ESLint extension has implemented a workaround for this, since they probably had the same problem at some point, I could investigate later how they handle it.The way I had to wrap the ChildProcess within a Promise is kind of ugly, and also the fact that we rely on a global variable
clangTidyProcess
to store info about the process status is not really sexy either. I think the whole tidy.ts module could be refactored to expose aClangTidy
singleton class, withrun
,kill
,getArgs
methods and aprocessStatus
attribute, and we could also certainly write our own small wrapper around 'execFile()` to Promisify it.I am willing to work on that refactoring, but before I do that I think we need to write some tests to ensure I don't break some functionality along the way. I would need your help on this, since I'm new to the extension and I do not really what the full extent of the features is. It would help if you could write down in a document the use cases that you would want to be covered by the tests, so that we can then code the corresponding test cases, or at least know what cases we should manually check.
Cheers !