search

alphapapa / magit-todos

Show source files' TODOs (and FIXMEs, etc) in Magit status buffer
GNU General Public License v3.0
729 stars 49 forks source link

Refactor scanner selection to check for availabilty (especially on remote systems) #178

Open mohkale opened 4 months ago

mohkale commented 4 months ago

Hi,

Looks like magit-todos doesn't re-calculate the scanner it uses based on which hosts its running on. If my local host has ripgrep but my remote does not magit todos fails when it can't find ripgrep.

alphapapa commented 4 months ago

Yes, this is mentioned in the readme.

mohkale commented 3 months ago

Finally, I'm not entirely sure about the name of the option, and maybe the option itself. It might be good to consider if this is the best way to handle the issue.

Objectively I think the sensible solution here would be no option but to check when you try to run a command whether it's available where you're trying to run it. You should ideally always do this. I think maybe removing the caching of the available scanner within magit-todos-scanner would be the best choice here. Leave it customizable as a symbol such as 'ag, 'rg etc. Anywhere we want to use a scanner we just call (magit-todos--choose-scanner) directly. If the user has manually selected a scanner which isn't available log a warning and skip (otherwise always search for the best one).

I think the performance optimisation of repeated executable-find calls is practically negligable and this would be the most robust solution going forward (for remote files executable-find is cached regardless, and on localhost it's so quick I don't see the harm).

For example, since a remote system might not have the same scanners installed, it might be good to address that problem in a unified way, e.g. testing for the scanner's presence and falling back to git diff, or disabling remote scanning entirely, etc.

I agree. I'd say always check for what's there. Maybe refactor the magit-todos-scanner option so it can be:

  1. A function (for legacy purposes).
  2. A symbol (for a defined scanner).
  3. A list of symbols (so users can set scanner preferences).
alphapapa commented 3 months ago

Yeah, that seems reasonable to me too.

I don't plan to work on this myself anytime soon, so patches welcome.