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

[Feature] Filter : include/exclude #1

Closed
rnsc opened this issue Jun 23, 2021 · 6 comments
Closed

[Feature] Filter : include/exclude #1

rnsc opened this issue Jun 23, 2021 · 6 comments

Comments

@rnsc
Copy link

rnsc commented Jun 23, 2021

Hello @Ana06 ,

I've looked at the code already and I see that the filtering implementation is leveraging regex to filter files.
I have a particular case that would require include a certain type of files but also exclude specific paths at the same time.
ie:

  • blah/test/something.yml
  • blah/not_at_test/something_else.yml
  • blah/another_folder/something.yml
  • blah/another_folder/something_not_a_yaml.j2

So I need to match all *.yml, but not *.yml that are in a test folder for example.
Now, while I undertsand I could make a convoluted regex to do that, I would also want to keep it simple.

Would you welcome a change where we change the filter by include and exclude both supporting regex, and add a bit more code logic?

I'd be willing to have a poke at it.

Thanks,

@micalevisk
Copy link

micalevisk commented Jun 27, 2021

This would be pretty cool to have.

Also, I'd suggest you change the new RegExp(`/${filter}\\b`, 'g') bellow to new RegExp(filter, 'g') (or am I missing something?) -- with this one could use filters like \.(js|ts)$ (instead of *\.(js|ts)$) and another more flexible pattern

const regex = new RegExp(`/${filter}\\b`, 'g')

And replace file.filename.match(regex) with regex.test(file.filename)

const files = response.data.files.filter(file => file.filename.match(regex))

@Ana06
Copy link
Owner

Ana06 commented Jun 28, 2021

@rnsc I think that is a great idea! Thanks for submitting it the issue. You are more than welcome to send a PR 😉

@Ana06
Copy link
Owner

Ana06 commented Jun 28, 2021

@rmed19 I would like to know your opinion about this, since the current implementation is yours. 😄

@agraul
Copy link

agraul commented Jun 29, 2021

What do you think about modeling the include/exclude filters after GitHub Actions' on.<push|pull_request>.paths? The options would be called paths and paths-ignore and accept lists of patterns instead of a string. It would make the change in #2 more complex, and I'm not sure if it's needed, but it might simplify workflow yaml files.

A use case that I can think of is that you only want to run an action when foo/bar.x is part of a commit and you also only want to run $test on foo/bar.

@rnsc
Copy link
Author

rnsc commented Jun 29, 2021

What do you think about modeling the include/exclude filters after GitHub Actions' on.<push|pull_request>.paths? The options would be called paths and paths-ignore and accept lists of patterns instead of a string. It would make the change in #2 more complex, and I'm not sure if it's needed, but it might simplify workflow yaml files.

A use case that I can think of is that you only want to run an action when foo/bar.x is part of a commit and you also only want to run $test on foo/bar.

Hey, thank you for the feedback!
Not sure I'm seeing the use case of accepting list of regexes though. Can't that be solved with a "simple" logic operation in the regex?

@agraul
Copy link

agraul commented Jun 29, 2021

My main motivation is to stay close to what already exists, the list of regexes is not something I came up with myself, it's GH's way™. However, it allows to have positive and a negative patterns grouped together:

with:
  paths:
    - 'sub-project/**'
    - '!sub-project/docs/**'

One aspect I didn't mention is that GH does not allow both paths and path-ignore at the same time, instead the syntax from this example is to be used (source).

Maybe it does not need to be this complex, the main advantage I see is that you could copy&paste the paths: list from the on: section to the with: section of this action.

Ana06 pushed a commit that referenced this issue Jul 27, 2021
* First commit with include/exclude filters

* Enable manual workflow runs

* Updated index.js

* add an action run with filters

* required include

* negative lookup

* updated index.js

* updated package.json

* new regex test

* updated workflow

* rollback packages update

* change the invert match logic

* fix regex

* testing dummy files

* better assignment

* remove dummy test files

* original packages.json

* fix yarn issue
Ana06 added a commit that referenced this issue Jul 27, 2021
[Feature] include/exclude filters (#1)
Ana06 pushed a commit that referenced this issue Jul 27, 2021
[Feature] include/exclude filters (#1)
@Ana06 Ana06 closed this as completed Jul 27, 2021
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

No branches or pull requests

4 participants