Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use real glob instead of simpleRegex #80

Closed
Trias opened this issue Nov 20, 2020 · 6 comments
Closed

use real glob instead of simpleRegex #80

Trias opened this issue Nov 20, 2020 · 6 comments
Assignees
Milestone

Comments

@Trias
Copy link

Trias commented Nov 20, 2020

I'm not sure if this is documented, but I was under the impression that the syntax used when providing fileMatch-Patterns for schemas ist a "glob"-Pattern. But it appers it is a "Simpleregex" defined by replacing certain characters as defined here: https://github.com/microsoft/vscode-json-languageservice/blob/master/src/utils/strings.ts#L34-L36

The key difference i noticed is that /**/ in a glob pattern matches zero or more folders, whereas in "simpleRegex" it matches one or more nested folders.

There is a package called glob-to-regex which appears to do the conversion respecting "globstar".

I'm happy to do the pull-request if you would approve this change.

@aeschli
Copy link
Contributor

aeschli commented Nov 30, 2020

As written in the API comment, currently we only support '*', not glob
I'm not opposed to allow glob as well but ideally implement it ourselves inside json-languageservice so I don't need to spend any time with legal approvals.

@Trias
Copy link
Author

Trias commented Dec 1, 2020

Understandable. Maybe we can use this code: https://github.com/microsoft/vscode/blob/master/src/vs/base/common/glob.ts?

Not sure tough how to integrate this code into into this library.

I realized that allowing glob pattern (obviously) is a breaking change, so probably we need to provide an alternative option in addition to fileMatch.

mifieldxu added a commit to mifieldxu/vscode-json-languageservice that referenced this issue Dec 14, 2020
@mifieldxu
Copy link
Contributor

hello! i'm new here. it turned out that the vscode-common repo doesn't actually have glob.ts in it. i implemented a solution to this with the vscode-anymatch package, which is just a fork of anymatch, but since it's a dependency for vscode-chokidar, i assume it's ok to use.

i also wrote some additional tests to make sure it works as expected, presuming that fileMatch is intended to be glob patterns.

i went ahead and submitted a pull request, but do feel free to do whatever you'd like!

@ssbarnea
Copy link
Contributor

Do we have any chance to get a some progress on this? Currently due to the very limited implementation it is impossible to correctly select schemas for some real world use-cases.

I am wondering it would be ok to just assume that **/ -> .* and * -> [^\/]*. Two stars should cross directory boundaries but one star should only match anything but the directory separator.

Do i have the impression that this got dead because Microsoft is unwilling to add an open-source license dependency? Minimatch is using ISC which is listed on https://spdx.org/licenses/ as one of the most liberal ones.

@aeschli
Copy link
Contributor

aeschli commented Mar 16, 2021

Sorry, I'm just swamped with other priorities. I'm not against the suggested changes, the node_module dependency is not a problem, just causes extra administrative work for which I didn't have had time yet.

ssbarnea added a commit to ssbarnea/vscode-json-languageservice that referenced this issue Mar 16, 2021
Fixes: microsoft#80
Co-authored-by: Sorin Sbarnea <sorin.sbarnea@gmail.com>
@aeschli
Copy link
Contributor

aeschli commented Apr 24, 2021

fixed by #93

@aeschli aeschli closed this as completed Apr 24, 2021
@aeschli aeschli added this to the April 2021 milestone Apr 24, 2021
@aeschli aeschli self-assigned this Apr 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants