Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: expand patterns in file #3

Merged
merged 1 commit into from
Mar 17, 2024
Merged

Conversation

314eter
Copy link
Contributor

@314eter 314eter commented Mar 17, 2024

patternsFile was not being used.

@ObserverOfTime
Copy link
Member

Made it a function.

@ObserverOfTime ObserverOfTime merged commit bcf090d into tree-sitter:master Mar 17, 2024
@314eter
Copy link
Contributor Author

314eter commented Mar 17, 2024

The patternsFile in action.getInput('files', { required: !patternsFile }) doesn't exist.

@ObserverOfTime
Copy link
Member

ObserverOfTime commented Mar 17, 2024

Ah shit. Fixed it.

@314eter
Copy link
Contributor Author

314eter commented Mar 17, 2024

Ok. Works now: tree-sitter/tree-sitter-ocaml#87

One peculiarity that is maybe not obvious for everyone, is that the files and files-list patterns are expanded separately. So you can't do files: examples/**/*.ext and then add exclusions in files-list. Maybe it's good to mention that in the documentation, or even recommend not to combine files and files-list.

@ObserverOfTime
Copy link
Member

Won't tree-sitter handle the exclusions?

@314eter
Copy link
Contributor Author

314eter commented Mar 17, 2024

No, we're passing a file with paths to tree-sitter, not patterns.

The behaviour of negated patterns depends on the order. They can only exclude files that were included before, not after. So if you want to combine files and files-list in a single "pattern collection", the order in which you do so becomes important.

I don't really see a use case for having both files and files-list, and the consequences can be confusing. Not allowing it at all is an option too.

@ObserverOfTime
Copy link
Member

I'll figure something out tomorrow.

@ObserverOfTime
Copy link
Member

Can you try v4?

@314eter
Copy link
Contributor Author

314eter commented Mar 18, 2024

As I said before, I don't think it's a good idea to combine the patterns like that.

With your new implementation, this will work as expected:

files: |
  examples/**/*.ext
files-list: files.txt
files.txt
---------
!examples/invalid1.ext
!examples/invalid2.ext

But this will not work

files-list: files.txt
files: |
  !examples/invalid1.ext
  !examples/invalid2.ext
files.txt
---------
examples/**/*.ext

@ObserverOfTime
Copy link
Member

That seems like a limitation of @actions/glob and there's nothing I can do about it.

@314eter
Copy link
Contributor Author

314eter commented Mar 19, 2024

That's not a limitation of @actions/glob, it's how git patterns are supposed to work.

But it does mean that the combination of files with files-list doesn't make sense. So it's a lot better to give a clear error message when someone tries to use them both (for which I don't see a use case), than to try to support it and do it badly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants