numtide / nix-filter

a small self-contained source filtering lib
MIT License
200 stars 17 forks source link

Make paths also match subdirectories #2

Closed robwhitaker closed 3 years ago

robwhitaker commented 3 years ago

This makes matchers that are generated for strings and paths treat the path as a prefix instead of as the entire path, allowing a path to also match its subdirectories and contained files.

This allows patterns like "src" to also match by default:

zimbatm commented 3 years ago

I would rather add a new matcher than change the default semantic.

In most cases, I want to precisely control all the files that get added. Otherwise, it's too easy for temporary files (generated by the editor or other tooling) to cause unnecessary rebuilds.

zimbatm commented 3 years ago

Also thanks for your work.

I want to expand a bit on the "Known limitation" section of the README which I believe motivated your work;

The built-in filter asks us to make that decision before entering that folder; if the filter returns false, the filter will stop recursing in that folder. Which is a good thing to avoid traversing parts of the filesystem we know are of no interest (like the .git folder). But sometimes it would be nice to be able to say "maybe", and then only add the folder to the output if any of the files are matching below. That's the limitation, and it's fundamental to how the filter works.

robwhitaker commented 3 years ago

This PR is addressing the comment, "This probably needs more work, I don't think that it works on sub-folders," more than the limitation noted in the README, though now I think I misunderstood the comment. It doesn't work on sub-folders like src/some-folder because of the limitation; is that right? It just never descends into src in the first place.

That said, I do think it's important to be able to match a folder and its children. It feels a little unintuitive to me that including "src/" doesn't include "src/Main.hs", for example, but I understand the reasoning for being conservative here. So maybe the path forward is to:

  1. Make an exactMatch matcher and clearly document that that is the default behavior if given a raw string or path, and
  2. Make this PR into a new matcher (inDirectory?)
robwhitaker commented 3 years ago

Regarding the limitation, I also have another PR in the works addressing that, which I'll open later today. I don't think it's quite ready to merge yet, but we can at least use it as a basis of discussion.

zimbatm commented 3 years ago

The other issue I had in mind is the reverse of this PR; if the user currently adds "src/sub/folder", it won't automatically match "src" and "src/sub". Currently, the user has to explicitly list all three of them. I think that one is not controversial.

For this PR, like you said there are two options;

robwhitaker commented 3 years ago

The other issue I had in mind is the reverse of this PR; if the user currently adds "src/sub/folder", it won't automatically match "src" and "src/sub".

This is actually something I've addressed already in the other PR I haven't opened yet, so I'll hold off on discussing it until that's up.


As for this PR, I think the way to go is to leave the default behavior as-is (i.e. src/ does not automatically match src/Main.hs) and turn this PR into a new matcher function (inDirectory) that explicitly includes everything within a directory.

I'll push that change sometime this evening.

zimbatm commented 3 years ago

Thanks a lot for your help, our both brains combined managed to find an elegant solution. I will merge this and deal with the consequences if there are any.