Negation in section name glob not working

[npetruzzelli]

I'm looking over the code in fnmatch.js to see whether or not I can feasibly fork this and take on a bug I've encountered, but I've hit a bit of a wall.

I've encountered some things I don't recognize neither from ES6+ nor TypeScript. I am a novice in typescript, but the file I'm looking at is plain JavaScript, so I am scratching my head.

https://github.com/editorconfig/editorconfig-core-js/blob/36259e5b44041d44ebf9b43b02d102b6bddddc3b/src/lib/fnmatch.js#L347

https://github.com/editorconfig/editorconfig-core-js/blob/36259e5b44041d44ebf9b43b02d102b6bddddc3b/src/lib/fnmatch.js#L510

https://github.com/editorconfig/editorconfig-core-js/blob/36259e5b44041d44ebf9b43b02d102b6bddddc3b/src/lib/fnmatch.js#L940

https://github.com/editorconfig/editorconfig-core-js/blob/36259e5b44041d44ebf9b43b02d102b6bddddc3b/src/lib/fnmatch.js#L961

Something in your build process must understand what these are, otherwise an error would be thrown somewhere. I don't see it being used at every for/switch/while instance, so it must be optional. Could someone point me in the right direction for where I can read up on it? I'm assuming it isn't something unique to this repository.

---

The bug I've encountered is that globs are not being handled correctly, specifically I am working on an EditorConfig file that wants to use negation. As far as I can tell, how you are building the path before passing it to your custom minimatch is contributing to it, but I'm sure it isn't as simple as that.

https://github.com/editorconfig/editorconfig-core-js/blob/36259e5b44041d44ebf9b43b02d102b6bddddc3b/src/index.ts#L47-L51

https://github.com/editorconfig/editorconfig-core-js/blob/36259e5b44041d44ebf9b43b02d102b6bddddc3b/src/index.ts#L104-L116

https://github.com/editorconfig/editorconfig-core-js/blob/36259e5b44041d44ebf9b43b02d102b6bddddc3b/src/index.ts#L158-L161

What I'm actually consuming is Prettier, which in turn, is consuming this module. I want to let editorconfig handle certain options, but if it can't match globs correctly, then I can't rely upon it.

My .editorconfig file is typically some (usually more complicated) variation of the following:

https://github.com/yeoman/generator-webapp/blob/cfed98e927438aad8fada4578ab0878b633544b1/app/templates/editorconfig

For the purpose of illustration, I will limit it to JSON files. I want to have a configuration for npm files and some other configuration for other JSON files.

# EditorConfig helps developers define and maintain consistent
# coding styles between different editors and IDEs
# editorconfig.org

root = true


[*]

# change these settings to your own preference
indent_style = space
indent_size = 2

# we recommend you to keep these unchanged
end_of_line = lf
charset = utf-8
trim_trailing_whitespace = true
insert_final_newline = true

[*.md]
trim_trailing_whitespace = false

[{package,package-lock}.json]
indent_style = space
indent_size = 2

[!(package|package-lock).json]
indent_style = tab
indent_size = 8

# ALSO TRIED:
[{*.json,!package.json,!package-lock.json}]
[{**/*.json,!**/package.json,!**/package-lock.json}]

The pattern that gets executed is usually something like: C:/path/to/my-project/{*,**/**/**}!/src/my-data.json which isn't a valid glob regardless of what the effective value for options.nonegate is. See also: https://github.com/isaacs/minimatch#comparisons-to-other-fnmatchglob-implementations

I haven't tried changing the order that sections appear in yet, but even if that works, negation (in this module) does not, so it is still buggy/unexpected.

As an aside, prior to my consumption through Prettier, I've been using EditorConfig plugins in Sublime and VSCode successfully for a long time and really appreciate all the work that has gone into the ecosystem. This is the first time I've found myself requiring some form of negation in a project. I'm a bit surprised I didn't find others having the same problem. No way I'm the first person to try using negation! ... right?

[jednano]

Yeah, that sounds a bit surprising to me too, considering all the cores run the same tests.

Anyway, the code that you couldn't figure out is called labels. They are used quite rarely in JavaScript, but they are useful in nestted loop situations where you want to break out of a specific loop.

I think the first step would be the figure out why the tests are passing in the first place. If you could somehow introduce a breaking test in https://github.com/editorconfig/editorconfig-core-test and THEN start worrying about a fix, that might make more sense and it would benefit all cores, not just the JS one.

Hope that helps! Thanks for being thorough.

[npetruzzelli]

Thanks Jed.

(English link to labels for those that want/need it)

I'll have a look at the core tests, but it will likely be a long time until I come back with an update. The bulk of my experience is JavaScript and a bit of PHP with some other languages here and there. I'm fine with stepping out of my comfort zone, it will just slow me down. A lot.

I'm not sure where I would begin to learn about how to write the tests or work with CMAKE. It looks like *.in files are just examples of .editorconfig files that will be consumed by a test framework of some kind. Since the section labels are globs I suspect I'll be starting here: https://github.com/editorconfig/editorconfig-core-test/tree/master/glob

[jednano]

Yes. That’s exactly what the .in files are for. yes, you’re on the right track.

[hildjj]

We're now using a standard version of minimatch, for better or worse, but I don't think this issue is related to minimatch. Instead, I think @npetruzzelli is on the right track that it's how we modify the glob before it gets executed.

minimatch's docs are a little... sparse, so it's hard for me to tell why this doesn't work:

minimatch(
  '/tmp/t/foo.json', 
  '/tmp/t/{*,**/**/**}/(!package).json', 
  { matchBase: true, dot: true, noext: true }
)

but if we can figure out what should go in the glob in this example, then we might be able to solve this.

The tests do NOT include any negations except in square brackets. Depending on what we figure out here, we might also want to add tests there, but I don't think it's necessarily a strict prerequisite.

[hildjj]

After poking at this some more, "just" setting noext: false in the minimatch options looks like it fixes this, without breaking any of the existing tests. I'm going to do a little poking around to make sure this doesn't cause a performance issue or break anything else we care about.

[npetruzzelli]

I don't remember what scenario caused me to open this years ago and I wish I had been able to contribute more, but if it is truly fixed, then I am glad to hear it!