fix for including excluded and vice versa

[adrianstephens]

I reported this earlier today, but then thought I'd have a go fixing it, and... behold!
I won't be offended if you don't use this - I am not a javascript expert.

[adrianstephens]

@microsoft-github-policy-service agree

[benibenj]

Thank you for looking into this. We try to follow the behaviour of npm packages to some degree and it's currently unclear to me if npm supports exclude patterns in their files property. If they do, then we should add support for this. If they don't we need to make sure to think this through. It might still make sure to support this as we do not support combining "files" and ".vscodeignore" which npm does.

If we decide on taking this we will need to add more tests here.

[adrianstephens]

It was unclear to me too - all I saw in the documentation was that "File patterns follow a similar syntax to .gitignore, but reversed.", and unofficial sources like https://medium.com/trabe/control-what-you-publish-inside-your-npm-packages-e3ec911638b8

I assumed not supporting files and .vscodeignore together was a deliberate attempt to avoid confusion; and if they both have the same expressive power why do you need the .vscodeignore at all (other than for legacy reasons)?

When you say 'we will need to add more tests' is the 'we' me or you? :)

[adrianstephens]

Just wanted to add, regardless of having exclude patterns in the files property, the real problem I was trying to fix was that I couldn't find a way to tell vsce that I wanted all the files in the 'assets' folder except the psds.

In .vscodeignore:

*
!dist/**/*.js
!assets/*
assets/*.psd

because the excludes are processed before the includes, it becomes

*
assets/*.psd
!dist/**/*.js
!assets/*

so the psd clause is overridden

[AshtonStephens]

@benibenj I added a file to the unit tests with explicit exclusion in the package.json file.

	"files": [
		"foo",
		"!**/exclude.me",  <-- I added
		"foo2/bar2/include.me",
		"*/bar3/**",
		"package.json",
		"LICENSE",
		"README.md"
	]

Then I added this file that's implicitly included by the foo path but is excluded based on the global filename match.

manifestFiles
├── LICENSE
├── README.md
├── foo
│   └── bar
│       ├── exclude.me <-- I added and is excluded
│       └── hello.txt
├── foo2
│   └── bar2
│       ├── ignore.me
│       └── include.me
├── foo3
│   └── bar3
│       └── hello.txt
├── logger.log
└── package.json

With these changes the tests all pass. Are there any other tests you'd like to see on this feature?

It definitely makes sense to be matching the npm behavior and I'm looking at their documentation and I see specifically the last bullet here states that doing exclusions with ! with the files parameter is undefined.

• "The consequences are undefined" if you try to negate any of the files entries (that is, "!foo.js"). Please don't. Use .npmignore.

I was going to say the documentation is inconsistent, but it actually consistently says this. AI tools, Gemini specifically, seem to very much disagree, so I think it's a matter of time before the npm team gets bug reports on this.

In the worst case we include this and we end up supporting something in vscode better than the actual tool, which is potentially misleading until the tool fixes it. On the other hand, it seems to me it's a matter of time until this is supported by npm, especially with Gemini being confused about it, and the ramifications of it becoming supported in npm and the vscode extension failing that is worse than the alternative.

The docs say including an excludes with a ! is undefined, and therefore exclusion falls into valid behavior when the behavior is undefined. So, future proofing the extension, I think it makes the most sense to consider merging it.

[AshtonStephens]

I reviewed and then added the test. Considering the test is very little and I did the reformatting I was going to request, I'm approving.

[wileyj]

lgtm

[benibenj]

I expanded the test to check if it works as expected. It does not.

If I exclude a folder and include files in that folder, the file will be included which it should not.
It should first consider all include patterns (do not start with !) and then based on the files which are included, should exclude all files matching the exclude patterns (start with !)

[adrianstephens]

I expanded the test to check if it works as expected. It does not.

I don't know if there are definitive rules about how include/exclude rules work, especially in a situation where there are two lists working opposite to each other. My feeling was that the intuitive thing was to take the clauses in order, so I want A, but I don't want B, but I do want C, etc. It was the sorting of clauses that made 'everything in assets except psd's' impossible to express.

[benibenj]

I strongly believe that the order in which the patterns are added to the files property should not impact the outcome. This is the same for .gitIgnore, .npmIgnore and .vscodeIgnore files. Making the outcome depending on the order, might make it a little more powerful, however, I fear that this would make it harder for users to understand and maintain. Defining your files should be a simple process.

[adrianstephens]

Using .vscodeIgnore in order to make it work like an include list I started with 'ignore everything' and then built up what I wanted; but the sorting prevented that from working. This change was initially just to fix that; adding '!' support for package.json's 'files' was secondary.

The .gitignore manual says, 'An optional prefix "!" which negates the pattern; any matching file excluded by a previous pattern will become included again.'. I don't know how accurately that matches their implementation, but it certainly sounds like they're maintaining the order.

Sorting precludes .vscodeIgnore from expressing 'everything in assets except psds'; the 'files' list could still do it, but not 'everything in assets except psds, but I do want icon.psd'.