Introduce IRelativePattern and adopt in DocumentFilter/FileWatcher/FileSearch #34695

bpasero · 2017-09-20T13:12:09Z

Creating this for discussion. Idea is to make it easier for extensions to work with patterns that should be relative to the workspace. We typically ask for a string for the pattern and now we could add an OR-type for a workspace relative pattern described by WorkspacePattern. The pattern will be matched against the provided base folder and can be relative (e.g. not starting with the workspace path or **).

export interface DocumentFilter {

	/**
	 * A language id, like `typescript`.
	 */
	language?: string;

	/**
	 * A Uri [scheme](#Uri.scheme), like `file` or `untitled`.
	 */
	scheme?: string;

	/**
	 * A glob pattern, like `*.{ts,js}`.
	 */
	pattern?: string | WorkspacePattern;
}

export interface WorkspacePattern {
	base: WorkspaceFolder;
	pattern: string;
}

Before doing more polish, @jrieken @dbaeumer for initial input.

jrieken · 2017-09-20T13:43:03Z

src/vs/vscode.d.ts

+		pattern?: string | WorkspacePattern;
+	}
+
+	export interface WorkspacePattern {


jsdoc missing

I wonder if we should also make it a relative pattern as we have in the lower level. A class like this

class RelativePattern { base: Uri; pattern: string; constructor(pattern:string, base: WorkspaceFolder | Uri) }

The ctor adds some util-sugar to it and making just a Uri allows for more generic use in extensions, e.g watch for file event in a certain folder (unrelated to a workspace folder)

Yeah I like that 👍

In addition I would add the type to createFileSystemWatcher. Eg. globPattern: string | RelativePattern

bpasero · 2017-09-20T16:14:41Z

@jrieken @dbaeumer latest changes include a new RelativePattern type as suggested. I think one challenge is that I cannot make this proposed API (at least I could not make it).

dbaeumer · 2017-09-21T07:19:13Z

Added my comments

jrieken · 2017-09-21T07:26:06Z

src/vs/workbench/api/node/extHostTypeConverters.ts

+
+export function toLanguageSelector(selector: vscode.DocumentSelector): LanguageSelector {
+	return selector as LanguageSelector;
+}


Do we actually need this? Is that an URI issue?

@jrieken yeah I think so, now I have to do the same for the watcher:

Try again, I have pushed some changes to URI and it should not be an issue anymore

bpasero · 2017-09-21T08:19:34Z

@dbaeumer @jrieken yeah I like adding this to the file watcher too because we have an actual use case for @joaomoreno who needs to watch per repository. And now with being able to use any URI as base, this should be possible.

I added it via 837ef11

joaomoreno · 2017-09-21T08:25:41Z

That's pretty cool. And I do believe that's how globs are supposed to work anyway.

I don't understand why we ever had globs which matched to absolute values. You'll find this pretty much nowhere else. In gulp, globs are always matched to a base. In zsh, they are always matched against the cwd, unless they start with /.

bpasero · 2017-09-22T06:12:50Z

@jrieken @dbaeumer I also decided to adopt this for findFiles to make this consistent and I have a use case for this from adopting the vscode-docker extension. I was also able to get rid of the no-op type conversions after merging latest from master in thanks to the URI changes.

jrieken · 2017-09-21T08:31:22Z

src/vs/workbench/api/node/extHostTypeConverters.ts

+
+export function toLanguageSelector(selector: vscode.DocumentSelector): LanguageSelector {
+	return selector as LanguageSelector;
+}


Try again, I have pushed some changes to URI and it should not be an issue anymore

jrieken · 2017-09-21T08:32:02Z

src/vs/vscode.d.ts

+		/**
+		 * A relative glob pattern like `*.{ts,js}`.
+		 */
+		readonly pattern: string;


Maybe no readonly for those two?

@jrieken yeah, pushed

jrieken · 2017-09-22T06:59:07Z

src/vs/vscode.d.ts

+		/**
+		 * A base to which the pattern will be matched against relatively.
+		 */
+		readonly base: Uri;


On a second thought this could also just be a string, right? We have the sugar ctor but technically we aren't globbing against a URI but against it path/fspath?

@jrieken yes it should be a string because this is about file paths, I changed that via f370693

jrieken · 2017-09-22T07:00:05Z

src/vs/vscode.d.ts

 		 * @param maxResults An upper-bound for the result.
 		 * @param token A token that can be used to signal cancellation to the underlying search engine.
 		 * @return A thenable that resolves to an array of resource identifiers.
 		 */
-		export function findFiles(include: string, exclude?: string, maxResults?: number, token?: CancellationToken): Thenable<Uri[]>;
+		export function findFiles(include: string | RelativePattern, exclude?: string | RelativePattern, maxResults?: number, token?: CancellationToken): Thenable<Uri[]>;


To avoid repetition we could consider something like

/** * Write Jsdoc once */ type GlobPattern = string | RelativePattern

@jrieken done via 0ba6fb6

jrieken · 2017-09-22T09:16:33Z

src/vs/workbench/api/node/extHostTypes.ts

+export class RelativePattern implements IRelativePattern {
+	base: string;
+
+	constructor(public pattern: string, base: vscode.WorkspaceFolder | string) {


fugly - mixing properties definitions like that

Yeah, oversight, fixed via 328f82a

bpasero · 2017-09-22T09:20:50Z

@roblourens it seems to me that the include/exclude patterns for findFiles actually are always matched in a relative way against the workspace they are coming from. E.g. I am seeing results coming back when passing in *.js, however I am not seeing results coming in when prefixing this pattern with the absolute path (e.g. /Users/bpasero/dev/myProject/*.js).

Given that, it seems we have a slight inconsistency with how we handle patterns in the API:

file watcher: absolute
document selector: absolute
search: relative to workspace

This makes it a harder to use the GlobPattern type for findFiles. It almost seems to me that we need something like:

type RelativePattern = string | RelativePatternWithBase

and then change findFiles method to only accept this RelativePattern type. It somewhat makes sense because a relative pattern can always also just be a string and if that is true we should match it against the workspace root(s).

@dbaeumer @jrieken thoughts?

jrieken

I am happy with the API changes, the rest I didn't really look at

bpasero · 2017-09-22T11:53:27Z

I clarified this in the JSDocs how patterns apply depending on where they are used, I think we should not make this any more complicated by introducing even more types. The reality is that string can always either be a relative glob pattern or an absolute glob pattern, we cannot enforce one or the other.

bpasero added 2 commits September 20, 2017 12:47

introduce IRelativePattern and use in extension API

53d5be9

💄

9af38ad

bpasero self-assigned this Sep 20, 2017

bpasero added this to the On Deck milestone Sep 20, 2017

bpasero requested review from jrieken and dbaeumer September 20, 2017 13:12

docs

cf3da26

bpasero mentioned this pull request Sep 20, 2017

Investigate broader support for WorkspaceFolder in the API (glob, doc filter) #34157

Closed

jrieken reviewed Sep 20, 2017

View reviewed changes

introduce RelativePattern

1877120

jrieken reviewed Sep 21, 2017

View reviewed changes

support RelativePattern also for file watcher

837ef11

bpasero modified the milestones: On Deck, September 2017 Sep 21, 2017

bpasero changed the title ~~Introduce IRelativePattern and adopt in DocumentFilter~~ Introduce IRelativePattern and adopt in DocumentFilter/FileWatcher Sep 21, 2017

bpasero added 3 commits September 22, 2017 07:58

also make findFiles support RelativePattern

c6435b3

Merge branch 'master' into ben/34157

5822b2b

less type conversion

0de9a75

bpasero changed the title ~~Introduce IRelativePattern and adopt in DocumentFilter/FileWatcher~~ Introduce IRelativePattern and adopt in DocumentFilter/FileWatcher/FileSearch Sep 22, 2017

jrieken reviewed Sep 22, 2017

View reviewed changes

bpasero added 4 commits September 22, 2017 10:48

add GlobPattern type and remove readonly

0ba6fb6

make base a string

f370693

fix setter access to RelativePattern

7c61869

fix npe when exclude is undefined

b8de125

jrieken reviewed Sep 22, 2017

View reviewed changes

fix findFiles: pattern seems to be matched against workspace always

280f77a

jrieken approved these changes Sep 22, 2017

View reviewed changes

bpasero added 2 commits September 22, 2017 11:38

💄

328f82a

clarify glob pattern matching

8739904

bpasero merged commit 3e9fa59 into master Sep 22, 2017

bpasero deleted the ben/34157 branch September 22, 2017 11:52

deysaikat95 mentioned this pull request Sep 25, 2017

Update from original (#1) #34941

Closed

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce IRelativePattern and adopt in DocumentFilter/FileWatcher/FileSearch #34695

Introduce IRelativePattern and adopt in DocumentFilter/FileWatcher/FileSearch #34695

bpasero commented Sep 20, 2017 •

edited

Loading

jrieken Sep 20, 2017

jrieken Sep 20, 2017

bpasero Sep 20, 2017

dbaeumer Sep 21, 2017

bpasero commented Sep 20, 2017

dbaeumer commented Sep 21, 2017

jrieken Sep 21, 2017

bpasero Sep 21, 2017

jrieken Sep 21, 2017

bpasero commented Sep 21, 2017 •

edited

Loading

joaomoreno commented Sep 21, 2017 •

edited

Loading

bpasero commented Sep 22, 2017

jrieken Sep 21, 2017

jrieken Sep 21, 2017

bpasero Sep 22, 2017

jrieken Sep 22, 2017

bpasero Sep 22, 2017 •

edited

Loading

jrieken Sep 22, 2017

dbaeumer Sep 22, 2017

bpasero Sep 22, 2017

jrieken Sep 22, 2017 •

edited

Loading

bpasero Sep 22, 2017

bpasero commented Sep 22, 2017 •

edited

Loading

jrieken left a comment

bpasero commented Sep 22, 2017

Introduce IRelativePattern and adopt in DocumentFilter/FileWatcher/FileSearch #34695

Introduce IRelativePattern and adopt in DocumentFilter/FileWatcher/FileSearch #34695

Conversation

bpasero commented Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpasero commented Sep 20, 2017

dbaeumer commented Sep 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpasero commented Sep 21, 2017 • edited Loading

joaomoreno commented Sep 21, 2017 • edited Loading

bpasero commented Sep 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpasero Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrieken Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpasero commented Sep 22, 2017 • edited Loading

jrieken left a comment

Choose a reason for hiding this comment

bpasero commented Sep 22, 2017

bpasero commented Sep 20, 2017 •

edited

Loading

bpasero commented Sep 21, 2017 •

edited

Loading

joaomoreno commented Sep 21, 2017 •

edited

Loading

bpasero Sep 22, 2017 •

edited

Loading

jrieken Sep 22, 2017 •

edited

Loading

bpasero commented Sep 22, 2017 •

edited

Loading