resources#isEqual ignores query-component

[jrieken]

The isEqual-utility ignores the query-component of uris, e.g it says http://duckduckgo?q=FOO equals http://duckduckgo?q=BAR which isn't correct. What should be excluded (optionally) is the fragment-component - see https://tools.ietf.org/html/rfc3986#section-6.1.

vscode/src/vs/base/common/resources.ts

Line 56 in f9465d1

export function isEqual(first: URI | undefined, second: URI | undefined, ignoreCase = hasToIgnoreCase(first)): boolean {

[jrieken]

I think we should (boldly) start to honour the query-components (since till today they are rare in our world). Alternatively (or depending on defaults additionally) we should make the isEqual configure, similar to the c#-uri-compare (https://docs.microsoft.com/en-us/dotnet/api/system.uri.compare?view=netframework-4.8). Tho, including the query in comparison should be the default

[jrieken]

Turns out that there is also this (which is a different identity):

vscode/src/vs/base/common/resources.ts

Line 18 in 0c1eb03

export function getComparisonKey(resource: URI): string {

My proposal is to add a function that can be configured to ignore the query and fragment parts. That function should then be used for getComparisonKey and for isEqual and all related functions.

Not sure about the defaults - spec leans towards ignoring the fragment but we have many, many usages that use toString() as identity and I have feeling that default behaviour of the comparator should honour that.

[aeschli]

Looking into what supporting query means for our resource.ts API.

• isEqual, getComparisonKey
• dirname, isEqualOrParent
• basename, extname
• joinPath, resolvePath, normalizePath, relativePath

One approach is to see the query as an addition the the file name (but not to parent path):

• dirname('/a/b/c.notebook?cell=1') === /a/b/
• basename('/a/b/c.notebook?cell=1) === 'c.notebook?cell=1''

Not clear what to do here:

• extname('/a/b/c.notebook?cell=1')

Worse:

• joinPath('/a/b?foo=bar', 'ff) === '/a/b?foo=bar/ff' -> result is an invalid URI

As the last example shows, this works against the URI syntax.

A better alternative would be to avoid the query and use an path syntax like 'a/b/c.notebook>1. The label formatter could work with that syntax too.

[aeschli]

Alternative is to continue to ignore the query parameter for all path related operations but preserve it:

• dirname('/a/b/c.notebook?cell=1') === /a/b/?cell=1'
• basename('/a/b/c.notebook?cell=1') === c.notebook or c.notebook?cell=1 (?)
• extname('/a/b/c.notebook?cell=1') === notebook
• isEqualOrParent('/a/b?cell=1', '/a/b/c.notebook?cell=1') === true

Not sure if that is of any use...

[jrieken]

Alternative is to continue to ignore the query parameter for all path related operations but preserve it:

Yes - I say that's the correct thing to do. Just like they ignore (already today) authority or scheme they must ignore query and fragment. For path operations it's an opaque piece of information

[aeschli]

But dirname('/a/b/c.notebook?cell=1') === /a/b/?cell=1 makes no sense.

[jrieken]

That you don't know and cannot say. If the query represents a search for instance, then it does makes to run it in each folder up the parent chain