expose doSearch for extensions #7445
Conversation
| throw new Error("RegExp query is required"); | ||
| } | ||
| return _doSearch(query.toString(), getCandidateFiles(), query); | ||
| } |
There was a problem hiding this comment.
A bunch of the search options are globals in the module, so the behavior in terms of what gets used will be awfuly inconsistent:
- Case-sensitive & regexp settings will be ignored (since this API always passes in its own regexp)
- It will use whatever exclusion filter was in place the last time a search was run, since it's initialized in the Enter-key handler
- It will use whatever subtree scope was in place the last time the search bar was open (even if it was canceled, unlike previous bullet), since that's initialized in
_doFindInFiles()
Actually, looking at what else _doFindInFiles() does, it appears that this function would break pretty badly if an existing set of search results was still visible in the bottom panel.
Overall it seems like to expose this in a reliable way we'd need to rework how state is stored in this module to be more compartmentalized. Doable, and a worthwhile cleanup anyway, but it requires much further-reaching code changes...
There was a problem hiding this comment.
I'll work on this little later and ping you for review @peterflynn
Do you think it'd be better to pass some kind of state object between functions or is there a cleaner solution when passing around too many variables?
There was a problem hiding this comment.
Putting the methods directly on some sort of state object would be another option potentially...
There was a problem hiding this comment.
A search class might be the best solution. We could export it and you can
even extend it to change what you don't want.
I actually did something similar but just for the results to be able to
share code between the find in files results and the replace all. But then
I figured that an easier solution would be to place a replace input and
button in the find in files results and make the replace all command
execute a find in file. This would also just work for replace in files.
|
Assign to myself and leaving open as reminder - I'll try to rewrite the code to use some kind of search class when I'll have the time. (If nobody will do that before me) |
|
Well I honestly don't have time to do this anytime soon right now. |
|
Yup, just noting this here so that we remember to think about this case if we end up refactoring it for any of the other cases. |
|
FYI, I'm currently planning to start doing some refactoring in the Find code in order to support the Replace Across Files feature. The first step is going to be to unify the FindReplace and FindInFiles input UIs, and I was planning to do that by separating out the find bar UI code entirely from those two modules, making the search code completely headless. (It won't be totally UI-free yet, because the FindReplace code will still interact with the editor and the FindInFiles code will still own the results panel at this stage, but it will at least be free of any UI code related to the find bar.) That's still probably not enough for what you want here because we'd also need to decouple the Find in Files engine from the results UI, but that's likely to happen in a later phase, where we generalize the current single-file Replace All feature to support Replace Across Files (which will probably involve unifying the Replace All panel with the Find in Files results panel, I think; I haven't looked at those parts of the code in detail yet to know if that makes sense, though). I'll keep in mind your use case here as that code evolves, and at some point might ping you to make sure it's going in the right direction for what you need. |
|
@zaggino - I've done a lot of refactoring in the nj/replace-on-disk branch that should help with what you want here. In particular, you should now be able to call I'm going to close this since that refactoring should be the right path forward - if you need more changes let me know. Thanks. |
|
(Note that in your case, it's probably as simple as |
|
Thanks @njx - I'll look at it later when'll have some time on my hands. |
ping @TomMalbran - i'd really like to use this to search for Git merge conflicts in a current project