Add {will,did}{Create,Rename,Delete}Files - #687

Dirk Bäumer (@dbaeumer) I've implemented a test for OnWillCreateFiles - can you check that what's here seems sane before I implement the others (which are largely a copy/paste of this with Create swapped for Rename/Delete).

I made the test server register statically + dynamically (slightly different paths) so both can be tested together (I don't have middleware tests yet, but they should be easy).

Any preferences for how to test the Did{Create,Rename,Delete} notifications went through? (They don't have a response like the Will{...} requests.

Thanks!

Edit: It might not be very obvious from the test, but the server creates an edit for a dummy reference file which just includes all the paths it received. Seemed like the easiest way for the client to verify the server got the expected event (as well as verifying the edit it passed through to waitUntil). If there's a better way, let me know!

Regarding the test question: you can also throw an error on the server side which will propagate back to the client and will make the send call fail. Normally this will make the test fail.

Regarding the test question: you can also throw an error on the server side which will propagate back to the client and will make the send call fail. Normally this will make the test fail.

I think this would prevent checking that calling waitUntil is being handled correctly? Or at least, require separate tests (some that throw with the file list, for testing the correct files were sent, and another that returns a known edit that is passed to waitUntil to ensure that's being handled/called correctly). Any preference?

Dirk Bäumer (@dbaeumer) any other feedback on this, or is it worth me implementing the other requests too? (they're mostly copy/pastes of this, though with the did ones being slightly simpler than the will ones as they're just notifications with no responses - though we will need a way to confirm in the test that the server received the notification?).

Danny Tuppeny (@DanTup) to be able to merge this we should have implementation for all notifications and requests. I will add my comments to the one implementation we have and the the rest should be copy paste.

Dirk Bäumer (@dbaeumer) sgtm! I always planed to implement them all before anything is merged - I just wanted to check you're happy with this one before creating the others. Thanks!

Danny Tuppeny (@DanTup) thanks for providing the PR.

Regarding pre-release: yes, either today or tomorrow. I still need to do some polish and I wanted to better understand the minimatch directory issue to see if we need to spec more.

Great!

Do shout if anything isn't clear or you want me to make changes. I went with the second option above (we suffix / on the folder URIs for matching, so both **/ and **/* will match all folders) so I think it should work as expected, though I'm not all that familiar with these glob patterns!

Thanks!

Danny Tuppeny (@DanTup) I played a litte with minimatch and I think we don't need to add the slash to the pattern since we have the pattern kind. I tested that for example:

let matcher = new Minimatch('a/**');
matcher.match('a/folder1/folder2/folder3');

evaluates to true.

So I would suggest that we do the following:

• if kind (e.g. folder or file) is undefined we take the pattern as is and match
• if kind is defined then we stat the URI and make sure that the type returned from the stat matches the requested kind.

This means we still need to do that stat but we don't need to tweak the paths. Is there something you can think of that doesn't make this work?

The filter method would then look like this:

	protected async filter(event: E, prop: (i: I) => code.Uri): Promise<E> {
		// (Asynchronously) map each file onto a boolean of whether it matches
		// any of the globs.
		const fileMatches = await Promise.all(event.files.map(async (item) => {
			const uri = prop(item);
			const path = uri.fsPath;
			for (const globs of this._globPatterns.values()) {
				for (const pattern of globs) {
					if (pattern.matcher.match(path)) {
						// The pattern matches. If kind is undefined then everything is ok
						if (pattern.kind === undefined) {
							return true;
						}
						const fileType = await FileOperationFeature.getFileType(uri);
						if (fileType === undefined || (fileType === code.FileType.File && pattern.kind === FileOperationPatternKind.file) || (fileType === code.FileType.Directory && pattern.kind === FileOperationPatternKind.folder)) {
							return true;
						}
					}
				}
			}
			return false;
		}));

		// Filter the files to those that matched.
		const files = event.files.filter((_, index) => fileMatches[index]);

		return { ...event, files };
	}

Tests are still green with that change. One thing we can debate is whether a failure of determining the file type should drop the event or sent it. Currently it is sent.

I have pushed the changes. Let me know if you see any problems with them.

I played a litte with minimatch and I think we don't need to add the slash to the pattern since we have the pattern kind

The case I was trying to cover was if the pattern ended with a /, eg:

// true
console.log(new Minimatch('a/**').match('a/folder1/folder2/folder3'));
// false
console.log(new Minimatch('a/**/').match('a/folder1/folder2/folder3'));
// true
console.log(new Minimatch('a/**/').match('a/folder1/folder2/folder3/'));

The middle one fails there, as the pattern has a trailing slash. I don't know whether that's reasonable or not, but some of my previous example patterns were like:

**{/,/*.dart}

If it's ok to say there shouldn't be trailing slashes on the patterns for directories (we can't enforce this easily ourselves if we're not doing the brace expansion), then I think the changes are fine. It would make sense to highlight this in the spec if so, to avoid catching people out.

One thing we can debate is whether a failure of determining the file type should drop the event or sent it. Currently it is sent.

I have no strong feeling on this, though since the server can easily drop it if it can't do anything with it (but can't handle what it doesn't get), maybe sending it makes most sense. Maybe worth being explicit about this in the spec whichever way you think is best?

OK. I do think we should cover console.log(new Minimatch('a/**/').match('a/folder1/folder2/folder3'));. I will look into fixing it.

Here is what I suggest we do: if the matching fails and kind is folder then we stat and if it is a directory we add a '/' to the end and rematch. The reasoning behind this is that the regular expression matching is a lot faster than stating the URI.

Does that makes sense?

See cd9b8e4

I can't think of any examples where that wouldn't work, so seems sensible to me.

The only question I have is whether minimatch cares that the trailing slash is forwards but the rest of the path might be backslashes now we're using .fsPath?

Minimatch actually doesn't care. But let me check again.

I used fsPath since we do use that in all other cases, even with document selector patterns. So this makes it at least consistent.

Minimatch actually doesn't care

Great, all sounds good to me then :-)

It does take care of it but it depends on the platform :-). Only under Windows it treats \ correctly and doesn't like '/' in a path. Need to see what we best do here to ensure everything works even with remote file systems.

Oh, I see. Can't we switch back to Uri.path and say that patterns always use /?

Using Uri.path is IMO confusing since VS Code always uses fsPath for patterns. This would result in strange behavior expecially under Windows with drive letters.

I did some more testing and it is actually OK. Under Windows:

let matcher = new Minimatch('a/**/')
> matcher.match('a\\folder\\folder\\')
true
> matcher.match('a\\folder\\folder/')
true
>

Which is OK. But what I will do after checking with what we do elsewhere is to replace \\ with / in the returned fsPath.

Done.

This would result in strange behavior expecially under Windows with drive letters.

Fair - I was thinking patterns would always be relative since they're implicitly constrained by the workspace, but that probably isn't a valid assumption.

Oh, one final thing - minimatch seems to be case-sensitive by default. I think we should document this, or state that they're case-insensitive and set nomatch?

// true
console.log(new Minimatch('a/**').match('a/folder1/folder2/folder3'));
// false
console.log(new Minimatch('A/**').match('a/folder1/folder2/folder3'));
// true
console.log(new Minimatch('A/**', { nocase: true }).match('a/folder1/folder2/folder3'));

Let me know if you want me to do any of that. Thanks!

Actually, that is a good point. We should add an options property to FileOperationPattern with a property ignoreCase: boolean.

Something like

interface FileOperationPatternOptions {
    ignoreCase: boolean;
}

Does that make sense to you ?

Yep! I can update the spec/client tomorrow if you'd like.

Should the options and/or ignoreCase be optional? And if so, what should it default to?

I made it optional. Looks like this

export interface FileOperationPatternOptions {

	/**
	 * The pattern should be match ignoring casing.
	 */
	ignoreCase?: boolean;
}

Default should be false as with all optional values.

I update the spec as well.

I added a comment here about a typo + making the default clear from the description:

943b2db#r44910295

But otherwise, all looks good to me :-)

Published new next versions.

Thanks! I rebuilt everything from the latest version of the spec in the Dart server, and my renames are all working as expected! 😀

Looking at the stuff in the v3.16 milestone here and for LSP, I presume there aren't any significant changes planned before it's finalised? I'd like to start preparing PRs for the server changes (for this + semantic tokens) but have been holding off until the spec was less likely to change.

Thanks again for your help + patience with this!

Thanks a lot for all your help with this. I don't plan to add any new features for 3.16. As soon as VS Code 1.52 ships I will release a new LSP version.

Perfect, thanks!

Out of interest - is there anything specific in VS Code 1.52 that is required for v3.16? I'm wondering whether I should be increasing the minimum version of VS Code when I switch to this latest client or not - I haven't noticed any issues using it with v1.51, but my testing has been quite focused on the changes in this PR.

The new version of LSP will require 1.52 because of the linked range editing support.

Ah, I see - I'll bump my minimum version once VS code releases then too. Thanks!