I don't think this should be done in-line while traversing. It is also possible to run through the list of entries in the ignore list, see which ones are directories and treat those as prefixes.

In fact, I'd like to convert the "list of exclude/include entries" into a function or class that encapsulates the behavior as soon as possible and pass that around.

I'm thinking something like:

const ignoreList = parseIgnoreList(rootDirectory, ...list of strings);

ignoreList will be either of type (path: string) => boolean, or be a class with a method of that type.

I would have that as an argument to listFilesRecursively().

The issue is that it wouldn't be very economical, fs calls-wise. I'm filling the exclude directory on the fly because I'm also checking the stat of the files I'm traversing.

Can we do these tests in memory?

I'm thinking of a test where we have preloaded this list of ignore patterns, and pushing a number of paths past it (either by means of shouldExclude() or by means of a new IgnoreList API)

That is effectively the same and easier to change/assert against than making a whole new fixture with "just these" files in it.

I ended up creating a createFsStructureFromTree function, and removing the previous fixtures (with the exception of demo-image/).

Does that work for you?

I'm not sure whether it's necessary to upgrade this to a feature of the assert library yet, especially since it doesn't really assert anything, will not be used by most of the construct libraries, and will never be JSII-able.

At least we can just keep this as a testing utility in the assets library until we definitely need it somewhere else, no?

I moved it to assert because I was using it both in assets and aws-ecr-assets. I could move it to assets instead, and expose it to make to available to aws-ecr-assets?

I see. Yes, I think put it in assets, and in fact you could import it directly from the test/ directory in aws-ecr-assets:

import fsUtils = require('@aws-cdk/assets/test/fs-utils');

It's not super nice, but I don't want test helpers to be part of the public API of a package. This usage would be okay by me since it's between 1st-party packages only. Would be nice to spend a couple of comment lines on it when you're making that change.

Done

I know reduce is sexy to use, but since you're not even using current accumulation res in your reducer, I don't really see the value. What's wrong with a simple for loop that returns the last set value?

I was originally using a map to have both pattern and patternIndex. I'll revert back to it.

Done

Does this function really need to be exported?

The name confuses me. Proposed name change: how about evaluateExcludePatterns.

Or in fact, I've been saying this a couple of times already: please put these related functions that operate on the same data structure in a class. They're not as independent as you would like.

class ExcludeRules {
  constructor(private readonly patterns: string[]) { }

  public excludeFile(filePath: string): boolean { ... }
  public excludeDirectory(dirPath: string): boolean { ... }
  
  private evaluate(pathName: string): [boolean, number] { ... }

}

I've switched to a class, that I'm still exporting to be able to test it. Same for your evaluate/my evaluateFile, it needs to be public.

Does this function really need to be exported?

I was exporting it to test it. It's the same now with the public method

Does this function really need to be exported?

I was exporting it to test it. It's the same now with the public method

I feel it would be cleaner if you made a function like pathComponents() (which given a path like a/b/c would return [a, a/b, a/b/c]) and iterated over that.

Again I feel a simple for loop would be more natural. If you really want to make it functional, why not something like:

const resultsAndPrios = pathComponents(relativePath).map(pc => shouldExcludePriority(rules,  pc));
const [shouldExclude] = pickHighest(resultsAndPrios, x => x[1]);
return shouldExclude;

Requires you to write some helpers, but to me is a lot clearer what is happening.

I did some refactoring, preferring for loops over reduces. I've only kept maps when I needed to know both the current array value and index.

In the case you're describing, shouldn't the function return false rather than `true?

Done

How about contradictory with the order reversed?

Done

Thanks for a thorough test suite!