Skip to content

🎁 Add search to file location methods in tasks#165156

Merged
meganrogge merged 11 commits intomicrosoft:mainfrom
babakks:add-search-to-file-locations
Nov 9, 2022
Merged

🎁 Add search to file location methods in tasks#165156
meganrogge merged 11 commits intomicrosoft:mainfrom
babakks:add-search-to-file-locations

Conversation

@babakks
Copy link
Contributor

@babakks babakks commented Nov 1, 2022

Fixes #160771

A new fileLocation method, named search, is added to problem matches. The usage of this file location method is as:

"problemMatcher": {
    "fileLocation": [
        "search",
        {
            "include": [ // (Required)
                "${workspaceFolder}/subdir1",
                "${workspaceFolder}/subdir2"
            ],
            "exclude": [ // (Optional)
                "${workspaceFolder}/subdir1/subdir-to-ignore",
                "${workspaceFolder}/subdir2/subdir-to-ignore"
            ]
        }
    ]

The include field is required, but the exclude is not.

To verify this PR, you can use babakks/vscode-verify-search-file-location repo as the test workspace.

meganrogge
meganrogge previously approved these changes Nov 1, 2022
View reviewed changes
Copy link
Collaborator

@meganrogge meganrogge left a comment

Choose a reason for hiding this comment

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

Very nice 👏🏼

@meganrogge meganrogge added this to the November 2022 milestone Nov 1, 2022
@meganrogge meganrogge requested a review from alexr00 November 1, 2022 16:38
alexr00
alexr00 reviewed Nov 2, 2022
View reviewed changes
Copy link
Member

@alexr00 alexr00 left a comment

Choose a reason for hiding this comment

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

I didn't see variables in the "includes" and "excludes" section get resolved when testing this.

src/vs/workbench/contrib/tasks/common/problemMatcher.ts Outdated
{
type: 'object',
properties: {
'include': { type: 'array', items: { type: 'string' } },
Copy link
Member

@alexr00 alexr00 Nov 2, 2022

Choose a reason for hiding this comment

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

When testing I didn't get any schema validation or suggestions for this object, which made it very easy to do

		{
			"include": "${workspaceFolder}\\extensions\\npm"
		}

which doesn't work because "include" should be an array.

Copy link
Contributor Author

@babakks babakks Nov 6, 2022

Choose a reason for hiding this comment

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

Added examples to the schema which will be rendered as suggestions in the runtime.

Also, for a better experience, changed the type of include/exclude arrays to accept single values as well; i.e., they're now string | string[].

Copy link
Contributor Author

@babakks babakks Nov 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

But, I'm not sure why validation didn't work.

@babakks
Copy link
Contributor Author

babakks commented Nov 6, 2022 •
edited
Loading

@alexr00 I've also added two other examples for the relative and autoDetect file location methods. The suggestions list now looks like this:

hover

@babakks
Copy link
Contributor Author

babakks commented Nov 6, 2022

I didn't see variables in the "includes" and "excludes" section get resolved when testing this.

@alexr00 I'm not sure I'm getting your point, correctly. When I tested, the variables (e.g., "${workspaceFolder}") were replaced with their values via the TerminalTaskSystem._resolveVariable method.

@babakks babakks requested review from alexr00 and meganrogge and removed request for alexr00 November 6, 2022 21:53
@babakks
Copy link
Contributor Author

babakks commented Nov 6, 2022

@meganrogge @alexr00 Couldn't ask you both for the review from the side links, so could you please re-review this?

@babakks
Copy link
Contributor Author

babakks commented Nov 7, 2022 •
edited
Loading

@meganrogge @alexr00 We can also make the include property optional too (default to ${workspaceFolder}). What do you think?

@babakks
Copy link
Contributor Author

babakks commented Nov 8, 2022 •
edited
Loading

It had a shortcoming that when the filename was more than a mere filename and extension (e.g., subdir/name.ext). Fixed that in the last commit; also updated the verification repo to include new tasks for this purpose.

@meganrogge
Copy link
Collaborator

meganrogge commented Nov 9, 2022

@meganrogge @alexr00 We can also make the include property optional too (default to ${workspaceFolder}). What do you think?

I like that idea - thanks for working on this

@babakks
Copy link
Contributor Author

babakks commented Nov 9, 2022

The include property is now optional and its default is "${workspaceFolder}".

@meganrogge meganrogge merged commit 7fc3b1f into microsoft:main Nov 9, 2022
@babakks babakks deleted the add-search-to-file-locations branch November 10, 2022 08:29
@babakks babakks mentioned this pull request Dec 6, 2022
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Dec 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@alexr00 alexr00 alexr00 left review comments

@meganrogge meganrogge meganrogge approved these changes

Assignees

@meganrogge meganrogge

Labels

None yet

Projects

None yet

Milestone

November 2022

Development

Successfully merging this pull request may close these issues.

The default "fileLocation": "relative" property for tasks does not work with npm tasks that use the "path" property

3 participants

@babakks @meganrogge @alexr00