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

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

Open
mohkale opened this issue Apr 23, 2024 · 3 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@mohkale
Copy link

mohkale commented Apr 23, 2024

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
Copy link
Owner

Yes, this is mentioned in the readme.

@mohkale
Copy link
Author

mohkale commented May 12, 2024

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
Copy link
Owner

Yeah, that seems reasonable to me too.

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

@alphapapa alphapapa changed the title Scanner doesn't check for scanner on remotes Refactor scanner selection to check for availabilty (especially on remote systems) May 12, 2024
@alphapapa alphapapa added enhancement New feature or request help wanted Extra attention is needed labels May 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants