save all modified files before running the checks or reloading the configuration

[ahus1]

This might fix #459.

I noticed the problem that the configuration wasn't reloaded even after clicking the button. This now saves all modified files to disk first, and then runs the check.

[jshiell]

That is a brilliant insight - it would explain why I could never reproduce it, as my auto-save settings are generally fairly aggressive, and I tend to window switch a lot when testing. Thank you! Even if it doesn't solve all of it, it definitely closes off one path to it 🤞

I'm going to take a little longer to think on this though - I'm tending towards that we should probably only save the relevant rules file, rather than everything, as otherwise we're shortcutting expected behaviour. And I know there are some use-cases where people don't want files saved unexpectedly (e.g. if they have something watching for FS changes).

[ahus1]

Hi - take your time, at least I now know some workaround.

Although saving all files might look a bit too much at first sight, some thoughts why it might be the right thing to do:

• when when pressing "reload", this should not only reload the configuration, but also other referenced files like a suppression file. When pressing reload, they should be saved as well.
• When running a scan on the project, the module, or changed files in the IDE, I don't know if CheckStyle picks them up form the IDE's memory, or from the filesystem. If it picks them up from the filesystem, it would be good to first save them all to the filesystem before triggering the scan.

While saving just the right files is hard, saving all of them is a lot simpler.

[jshiell]

when when pressing "reload", this should not only reload the configuration, but also other referenced files like a suppression file. When pressing reload, they should be saved as well.

This is determinable, given we have to do it at runtime, but you're not wrong on it being a bit of a pain in the arse.

When running a scan on the project, the module, or changed files in the IDE, I don't know if CheckStyle picks them up form the IDE's memory, or from the filesystem. If it picks them up from the filesystem, it would be good to first save them all to the filesystem before triggering the scan.

There's actually a joyous path of writing out temp files for unsaved documents (in the correct package structure), to ensure we can scan them. So this, at least, should be taken care of.

One fallback might be to offer a pop-up on reload saying "unsaved files, do you want to save", with a checkbox to do it automatically in future 🤔 Presumably most would click "do it", and those with odd requirements have an opt-out.

[jshiell]

Sorry, this has all been delayed due to the issues with the mutli-selection PR, which are hopefully now sorted 🤞

My thinking is currently that the perfect answer is a checkbox to opt in, but it's probably too much hassle. So what I instead suggest is that we do this only for the "reload configuration" action - the other actions have logic to create on-disk copies without saving (which we need for the inspection if nothing else), so those should already work as intended - and people taking the reload action are explicitly resyncing with the FS, so it shouldn't be so surprising.

[jshiell]

Hmm, I also suggest we template a sub-method for the base class, rather than relying on people calling super. I'll try have a look at this in the next week or so, maybe this weekend if guests don't steal all of my time 🤞

[ahus1]

Thanks you for these guiding comments. As this is now only necessary for ResetLoadedRulesFiles, this make the PR a lot simpler, and there's no need to call user any more.

I pushed some changes.

[jshiell]

Smashing - thanks very much!