microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
163.59k stars 29.03k forks source link

[json] jsonValidation fileMatch is confusing when used with URIs #67191

Open johnspurlock opened 5 years ago

johnspurlock commented 5 years ago

Steps to Reproduce:

  1. Create an extension that registers a text document content provider for a custom scheme, say custom. Implement the provider to return a json object as text.
  2. Declare a jsonValidation in package.json with a fileMatch of custom://*.todo.
  3. Open a text document for a uri under that custom scheme with path ending in something.todo, and also a defined query or fragment [1], then set text document language to 'json', and show it.
  4. Expect the json schema validation to occur on the virtual file.
  5. Note that it does not.

It appears the fileMatch matches on the entire uri, including the query and the fragment! You can verify this by changing the fileMatch to custom://*.todo?*

It should probably only match on the path.

[1] e.g. custom:/path/to/something.todo?foo

aeschli commented 5 years ago

Yes, it's implemented as a match against the URI string. I'd rather not complicate things here.

When we compare resource identifiers we also use the whole URI string, why would we, when matching just ignore these? To me, query and fragment feels out of place on a resource URI. But of course you as the resource provider are free to define how they look. If you go with query I think then you need to define the pattern accordingly.

johnspurlock commented 5 years ago

Ok, it was surprising to me since 1) the editor tab chooses to use the path (last segment) as the "file name" (ignores the query) for virtual files and 2) it's called fileMatch

I suppose it's safer to leave things as is, considering existing extensions that assume this behavior. Also would rather have an actual regex/glob pattern matcher here at some point, so perhaps this could be addressed as part of that.

aeschli commented 5 years ago

Yep, agree, fileMatch is confusing. It was first designed only for extension relative paths, later extended to URIs. Should probably change to a different property (uriMatch?) that allows more URI sematic matching.