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

Improve include/exclude search in workspace #9307

Merged
merged 1 commit into from
May 11, 2021

Conversation

alvsan09
Copy link
Contributor

@alvsan09 alvsan09 commented Apr 6, 2021

The following descriptions apply to the user input for the fields
'Files to include' and 'Files to exclude' for the 'Search in
workspace' feature.

Enhancements:

  • Handle provided relative paths e.g. ./README.md, README.md to only
    apply the specified file.
  • Includes are limited to specific folder e.g. './test' should
    not include './dir/path/test'.
  • Input strings (i.e. not resolving to relative paths) are converted
    to two globs in order to include results of a resulting file and
    include files under a resulting folder i.e. resolved globs are
    **/string and **/string/*.
    E.g. input file '.snaphot' shall include a file '.snapshot' or if
    it's a folder it should apply to all the files underneath it.
  • 'Files to include' can reference folders outside the workspace

Fixes: #8469

The search strategy offered by the search in workspace server
(RipgrepSearchInWorkspaceServer.search) is changed as follows:

By default, sets the search directories for the string WHAT to the provided ROOTURIS directories
and returns the assigned search id.

The include / exclude (options in SearchInWorkspaceOptions) are lists of patterns for files to
include / exclude during search (glob characters are allowed).

include patterns successfully recognized as absolute paths will override the default search and set
the search directories to the ones provided as includes.
Relative paths are allowed, the application will attempt to translate them to valid absolute paths
based on the applicable search directories.

Signed-off-by: Alvaro Sanchez-Leon [email protected]

What it does

For strings provided for files to include / exclude

  • Checks if the 'include' strings provided are valid paths to the available root folders and if so convert them to glob patterns
  • If the include strings are valid paths, replace the root paths with the resulting paths to be consumed by the 'ripgrep' executable.
  • Consider each string not resolving to sub-folders as a pattern to either a file or folder, therefore generating two glob entries for each.

How to test

4 new unit test cases provided

Manual testing can be performed as follows:

Using theia repo as an example directory structure:

  1. Search to a relative file path of workspace root(s)
    • Search pattern: cloud (produces a handful of matches)
    • Enter 'Files to Include' text to: ./README.md (verify that only one file is considered)
  2. 'Files to include' can refer to files
    • Search pattern: example
    • Enter 'Files to include' to: .eslintcache (verify that multiple matches are reported)
  3. 'Files to include' can refer to folders
    • Search pattern: example
    • Enter 'Files to include' to: test (verify that multiple matches are reported)
  4. Searching limited to a given subfolder
    • Search pattern: example
    • Enter 'Files to include' to: */doc/ (note that matches in two different folders are found i.e. 'doc' and 'packages/plugin-ext/doc'
    • Replace 'Files to include' to doc (note that this now resolves matches for a single folder i.e. 'doc')
  5. From the explorer view select a file or folder and from the context menu select 'Find in folder', press enter, and verify the results are as expected.
  6. Using the 'Files to include' box, enter absolute paths to external folders, validate expected results.

Review checklist

Reminder for reviewers

@vince-fugnitto vince-fugnitto added the search in workspace issues related to the search-in-workspace label Apr 6, 2021
@alvsan09 alvsan09 force-pushed the siw_enhancements branch 2 times, most recently from d2e3b53 to 946d53e Compare April 9, 2021 13:06
@alvsan09 alvsan09 force-pushed the siw_enhancements branch 2 times, most recently from e5fb8e9 to b0e3dbb Compare April 14, 2021 16:41
@alvsan09 alvsan09 force-pushed the siw_enhancements branch 3 times, most recently from bf41d62 to 3132c25 Compare April 15, 2021 16:42
Copy link
Contributor

@DucNgn DucNgn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some suggestions 😄
Let me know what you think!

@alvsan09
Copy link
Contributor Author

Hi @vince-fugnitto , @marechal-p , @dukengn,
I have addressed all comments raised so far, please let me know if there is anything missing.
Regards
/Alvaro

@alvsan09 alvsan09 force-pushed the siw_enhancements branch 2 times, most recently from 17b9d4a to 34f535e Compare April 23, 2021 21:34
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alvsan09 I only have minor comments left regarding the source-code. When addressed can you please squash the commits so that I can verify the final commit message and perform functional tests?

@alvsan09 alvsan09 force-pushed the siw_enhancements branch 2 times, most recently from a309ada to 10538dd Compare April 27, 2021 18:22
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed that the search works for various use-cases 👍

  • searching like today on master
  • prefixing search with ./ for narrower results
  • searching outside the workspace (similarly to vscode)
  • no visible regressions to search behavior

@alvsan09 alvsan09 force-pushed the siw_enhancements branch from 10538dd to 1991e84 Compare May 4, 2021 12:48
Enhancements:
* Search with 'Files to include' specifying a relative path
  e.g. './README.md' will only consider existing files from workspace
  roots e.g. '${ROOT1}/README.md', '${ROOT2}/README.md'.
* 'Files to include' are limiting the search to specific folders
  e.g. './test' should not include '${ROOT}/dir/test'.
* Include/Exclude strings (i.e. not resolving to relative paths) are
  converted to two globs in order to include results of a resulting
  file and include files under a resulting folder i.e. resolved globs
  are `**/string` and `**/string/*`.
  e.g. input file '.snaphot' shall include a file '.snapshot' or if
  it's a folder it should apply to all the files underneath it.
* Absulte path(s) given as include patterns override the search in
  workspace to search under the given file/folder path(s).
* Files opened in editor will search the contents in editor so the
  search will exclude these files when searching the file system.
* Include / Exclude file strings starting with './' (linux) or
  '.\' (windows) will be applied as absolute patterns relative to the
  applicable search paths (even if they include glob patterns).

Signed-off-by: Alvaro Sanchez-Leon <[email protected]>
Co-authored-by: marechal-p <[email protected]>
@alvsan09 alvsan09 force-pushed the siw_enhancements branch from 1991e84 to 871ec2e Compare May 7, 2021 21:16
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed that the changes still work well functionally with the latest updates 👍

@vince-fugnitto vince-fugnitto merged commit 3415be0 into eclipse-theia:master May 11, 2021
@vince-fugnitto vince-fugnitto added this to the 1.14.0 milestone May 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
search in workspace issues related to the search-in-workspace
Projects
None yet
Development

Successfully merging this pull request may close these issues.

siw: discrepancy in the search results with 'include' pattern beginning with './'
4 participants