Open ssbarnea opened 3 years ago
I like the idea of switching to either minimatch
or micromatch
. I found a github gist of the differences and it seems that micromatch is slightly better. For reference the json language service is moving towards switching their file matching as well to fix issues like this
I would also point out that minimatch
is no longer being maintained. I would suggest using https://github.com/micromatch/micromatch
I tried to make use of micromatch but I had some problems making use of it because is JS instead of TS and I do not yet master the tricks of making it importable without upsetting eslint.
For reference: https://github.com/redhat-developer/yaml-language-server/compare/master...ssbarnea:micromatch?expand=1 -- imaybe someone knows how to correctly use micromatch in TypeScript as current code does not compile.
@ssbarnea Change YAMLSchemaService
is not enough as it extends JSONSchemaService
which still use old style patterns.
To complitly finish https://github.com/redhat-developer/yaml-language-server/issues/412 any way we need to write own implementation. As during JSON parsing we are losing offsets of JSON Schema tokens and I cannot implement navigation to particular part of JSON Schema which cause error as I have only JS object and doesn't know where in JSON Schema it placed.
I can start fixing this by replacing JSONSchemaService
with our code in YAMLSchemaService
and use micromatch
for fileMatch
patterns, and then return to https://github.com/redhat-developer/yaml-language-server/issues/412
@JPinkney WDYT?
I got some fresh feedback on https://github.com/microsoft/vscode-json-languageservice/issues/80#issuecomment-800225920 so I guess there is a change to fix this at the source! If we do it before it gets cold again.
Ideally we can get the PR merged on vscode-json-languageservice and then just make sure our code works with it.
I can start fixing this by replacing JSONSchemaService with our code in YAMLSchemaService and use micromatch for fileMatch patterns, and then return to #412
Yeah, that makes sense. I haven't taken a look yet, would there be a lot of changes required?
I haven't taken a look yet, would there be a lot of changes required?
No, but I don't think that it will requires a lot of changes. Going to provide a draft PR with that.
@ssbarnea @JPinkney @joshuawilson I create the PR with minimatch
usage.
Could you look on it?
Alternatively or additionally to blobs, it would be nice if regular expressions could be used to match file paths, and it was already implemented at one point https://github.com/redhat-developer/yaml-language-server/commit/d4a05e3dd72f55c53f1b0325c521a58f688839c9
Upstream already adopted minimatch and https://github.com/redhat-developer/yaml-language-server/pull/448 is supposed to make it available to use.
I am not sure if further changes are needed or if we can consider this already done.
I think we might still might have to adapt https://github.com/redhat-developer/yaml-language-server/blob/master/src/languageservice/parser/isKubernetes.ts#L15 because it looks like it still uses the old file pattern association
@ssbarnea I assume this issue is still open because you are not able to get your use case glob pattern scenario working?
If I remember well microsoft already switched to using a more modern pattern matcher few months back. I was waiting for a yaml-language-server to catch-up and a new version to be released as part of vscode-ansible in order to update my patterns. Does the latest release include pattern matcher updates? If so I could give it another go.
We use new matters with https://github.com/redhat-developer/yaml-language-server/pull/448 and it was included in 0.19.0
release.
Now vscode-json-languageservice
switched from minimatch
to globregexp
https://github.com/microsoft/vscode-json-languageservice/commit/3c8b871f8e08889cbaabe117e1f9186f0ed73e4e in lates release.
Should we also switch?
@(mini|micro)match
support extglobs which is precisely what I came here to request before finding this issue.
"yaml.schemas": {
"github-issue-forms": ".github/ISSUE_TEMPLATE/!(config).yml",
"github-issue-config": ".github/ISSUE_TEMPLATE/config.yml"
}
I am still trying to find a negating pattern that works for making these exclusive (avoid having both active at the same time):
"file:///Users/ssbarnea/c/a/schemas/f/ansible-tests-meta.json": [
"tests/**/meta/main.yml"
],
"file:///Users/ssbarnea/c/a/schemas/f/ansible-meta.json": [
"**/meta/main.yml",
],
I tried various ways but not managed to negate the second one.
The full path would be like /Users/ssbarnea/c/a/schemas/test/tests/integration/role_foo/meta/main.yml
and because we have tests
before, I do not want to select the second schema.
I tried !(tests)**/meta/main.yml
, !(tests/)**/meta/main.yml
and it seems that it always picks both. I used Reload Window to ensure that it reloads it.
Apparently internally we use regex and if we want to exclude specific text from being include a regex like ^((?!tests\/).)+\.yml
should work.
That makes me believe that we could attempt to convert !(foo/)
into ^((?!foo\/).)+
when creating the regex. The ^
is needed as we do not want to accidentally match as a substring.
Am I going into the right direction with this or we should better try to replace our implementation with the same one that vscode is using?
Based on https://github.com/microsoft/vscode/blob/b35bc5e02109682f50ec2ff4ef6537c50ed75cb9/src/vs/workbench/api/common/jsonValidationExtensionPoint.ts#L29 they officially support !
as a marker for making a pattern negative.
I am inclined to say that we should do the same, even before switching to the same implementation. If glob pattern starts with !
it should be transformed into a negative match.
@evidolob @msivasubramaniaan WDYT? Should I try to patch our implementation to do it?
@ssbarnea any update on this? also running into this issue.
Is your enhancement related to a problem? Please describe.
At this moment fileMatch pattern is suffering from several limitations that make it impossible to write patterns that correctly identify Ansible file types.
Describe the solution you would like
To see a full list o patterns that can reliably be used to identify Ansible file types, look at https://github.com/ansible-community/ansible-lint/blob/master/src/ansiblelint/config.py#L14-L33
There are two missing feature that prevent that:
*
expands as anything instead of current file.foo/*.yml
currently also matchingfoo/bar/*.yml
when instead it should not. The correct way to do it would be withfoo/**/*.yml
orfoo/*/*.yml
, where the second one requires one sublevel folder but the first one allows none, one or many subfolders feel (aka the recursive one).{...}
, so a pattern like"**/{host_vars,group_vars,vars,defaults}/**/*.{yaml,yml}"}
must be exploded to 8 different patterns in order to work with current fileMatch. You can easily see how we can endup with very long and hard to maintain patterns.Describe alternatives you have considered
While for the second missing feature we can probable live with the extra inconvenience, there is no solution for the first one. That is directly affecting Ansible because Ansible has a layout where nested patterns are needed and we must be able to control the matching to a single folder depth
Additional context
The given examples of patterns are based on https://facelessuser.github.io/wcmatch/ which is a python matching library but they are not unique to them. At least the globstar (
**
) is part of the extended glob syntax. Based on https://en.wikipedia.org/wiki/Glob_(programming)#cite_note-bashpat-10 it seems that there are at least two popular JavaScript implementation that should support it:minimatch
(npm) andmicromatch
(babel/yarn).