search

IvanFon / super-labeler-action

A superpowered issue and pull request labeler for Github Actions.
GNU General Public License v3.0
22 stars 12 forks source link

feat: allow pass flag/modifires in regexp pattern #1

Closed jbinda closed 4 years ago

jbinda commented 4 years ago

Hi @IvanFon thanks for great work on the super-labeler-action ! 🥇

I have started to use it recently because it covers all of use case that I need to proper label issues and PRs.

However I was unable to pass flags/modifires to titleMatches pattern shorthand. If you know the way how to pass the pattern like this "pattern": "/^wip:/i" (the goal here is to match case insensitive word and main issue is that in JSON it should be wrapped in double quotes) please let me know (also then this PR is unnecessary ;) ). .

Like I mentioned above I was struggling with RegExp pattern to pass /i flag which make the check case insensitive. Checking the codebase I noticed that RegExp object takes only first parameter with pattern and second parameter with flags is omitted. See details with example below:

```jsx const titleMatches = ( condition: ConditionTitleMatches, issue: IssueProps | PRProps, ) => { const pattern = new RegExp(condition.pattern); //omited flag parameter return pattern.test(issue.title); }; ```

Also passing shorthand regexp as string in JSON is problematic( see this thread on stackoverflow). I have used proposed solution and make PR to allow pass shorthand regexp including flags/modifirers.

With this PR you will be able to replace

{
  "type": "titleMatches",
  "pattern": "^(wip|WIP):"
}

with this one

{
  "type": "titleMatches",
  "pattern": "/^wip:/i"
}

Which seems to be not big beneficial in above case. However it can cover all possible option of capitalising the wip word if necessary. In my case I don't care about the letter capitalising in issue ticket - more important is to applied a label. I prefer to edit the title at some point that triaging and adding label

I think the alternate way might be also passing additional flag prop in matchers object. See details below

```jsx export interface ConditionTitleMatches { type: typeof TYPE; pattern: string; } ``` to ```jsx export interface ConditionTitleMatches { type: typeof TYPE; pattern: string; flag: string; } const titleMatches = ( condition: ConditionTitleMatches, issue: IssueProps | PRProps, ) => { const pattern = new RegExp(condition.pattern, condition.flag); //pass flag parameter return pattern.test(issue.title); }; ```

Please share your thoughts about this !

IvanFon commented 4 years ago

Thank you for the pull request! :)

This is definitely an important feature. I'm just thinking about maintaining backwards compatibility. From some quick experimentation, I noticed some weird cases:

> processRegExpPattern('/wip|WIP/')
/\/wip|WIP\//
> processRegExpPattern('\/wip\/$')
Uncaught SyntaxError: Invalid flags supplied to RegExp constructor '$'

It's pretty late right now, I'll think about it a bit more tomorrow. Having separate pattern and flag properties might simplify it, but it would make writing the patterns a bit more tedious. ¯\(ツ)/¯

Again, thanks for the contribution! On an unrelated note, could I ask how you found this action? I see a few people have starred it, I'm just curious where people are finding it, as I haven't really shared it anywhere aside from the marketplace and the Github Hackathon.

jbinda commented 4 years ago

Hi ! Thanks for review it

This is definitely an important feature. I'm just thinking about maintaining backwards compatibility. From some quick experimentation, I noticed some weird cases:

processRegExpPattern('/wip|WIP/')
/\/wip|WIP\//
processRegExpPattern('\/wip\/$')
Uncaught SyntaxError: Invalid flags supplied to RegExp constructor '$'

I'm not sure if those weird cases do not comes from wraping regexp in quotes (single quotes in that case) without backslashing the special characters. The / in regexp is similar to " for strings.

See below comparison. RegExp.source will return the same result for both

1. We will be looking exact match of WIP or wip across given string

const patternString =  "wip|WIP" 
const patternRegex =  /wip|WIP/

console.log(RegExp( patternString).source)   // wip|WIP
console.log(RegExp( patternRegex).source)  // wip|WIP

I assume that we want to find wip or WIP across the title. Then outside / is not necessary (see above). Wrapping it in single quotes like this '/wip|WIP/' will produce incorrect regex and it will look for /wip or WIP/.

However if you ment here to look /wip/ or /WIP/ the proper string regexp wiill be /\/(wip|WIP)\//. Also the round brackets needs to be passed to open the group. Without brackets you will be looking still /wip or WIP/.

Please check this example

2. We will be looking exact match of /wip/ from the end of given string

const patternString =  "\\/wip\\/$"  // you need backslash special character to produce the same source - thats why double backslash
const patternRegex =  /\/wip\/$/

console.log(RegExp( patternString).source)   //   \/wip\/$
console.log(RegExp( patternRegex).source). //   \/wip\/$

Wrapping it in single quotes like this '\/wip\/$' will produce incorrect regex and it will look for /wip that's why you need to backslash special character (in this case backslash) and that's why finally you have double backslash

So IMO the proper way to pass it will be \\/wip\\/$ or \\/(wip|WIP)\\/$ if we want to extend by searching /wip/ or /WIP/. See regexp example here

Summarising all above. The issue is that we want to pass RegExp from JSON so we need to use proper backslashing on special characters because we only can pass a string (regexp is not allowed as it is in JS). Regexp object will parse string only if it has proper backslashed special characters inside.

Adding modifires to regexp is simple because you just need to add the / wrapping whole expression and then add flag at the end like this `/ ... /i

So the conclusion from all above is that you need to backslash all special characters to get the proper regex parsed from string that we passed in JSON config.

Woow that was a lot.. Hope it make sense and you be able to get my point.

About backwards compatibility can't you release new major version ? Also maybe update the README.md that there is new rules to pass regexp. However still I think proposed way is the proper one to pass regexp through string...

One more question for the end ? Do you have possibility to test it locally ? What tool are you using ?

jbinda commented 4 years ago

Again, thanks for the contribution! On an unrelated note, could I ask how you found this action? I see a few people have starred it, I'm just curious where people are finding it, as I haven't really shared it anywhere aside from the marketplace and the Github Hackathon.

Unfortunately it wasn't the top google search results (hope it will increase iis rank soon ! ). Although I did some research on labeling because I need one tool which combines all of use case that I need. I started to test all Github Laleler actions one by one to choose the one fits me most. The choice was super-labeler because it allows me what I want, see below liist:

Also the cool thing in super-labeler is that I can store labels config in one JSON and the label will be created if not exist (it is powerful feature to have consistent labeling across few repos - my company maintain actively some OpenSource repos and we are aiming to be consisten in processes to manage and maintain then)

Before I started to use your repo I need to use 3 separate packages for that. Now I use only yours (I will post you side not e in separate issue because I have some problem with matching the files in PR).

I still have some use case that I would like to have and would like to raise some other PR in the future if you don't mind :)

Do you have any new feat that you would like to introduce in near future ? I will be glad to have also

jbinda commented 4 years ago

Rock and roll ! 🎉 🎉 Thank for merging ! :)

IvanFon commented 4 years ago

Summarising all above. The issue is that we want to pass RegExp from JSON so we need to use proper backslashing on special characters because we only can pass a string (regexp is not allowed as it is in JS). Regexp object will parse string only if it has proper backslashed special characters inside.

Adding modifires to regexp is simple because you just need to add the / wrapping whole expression and then add flag at the end like this `/ ... /i

So the conclusion from all above is that you need to backslash all special characters to get the proper regex parsed from string that we passed in JSON config.

Woow that was a lot.. Hope it make sense and you be able to get my point.

Ah, thanks, thanks!

About backwards compatibility can't you release new major version ? Also maybe update the README.md that there is new rules to pass regexp. However still I think proposed way is the proper one to pass regexp through string...

I'll update the README. Is this a breaking change? processRegExpPattern should detect if you're using the old style, right?

One more question for the end ? Do you have possibility to test it locally ? What tool are you using ?

My testing setup isn't very good :(

Unfortunately it wasn't the top google search results (hope it will increase iis rank soon ! ). Although I did some research on labeling because I need one tool which combines all of use case that I need. I started to test all Github Laleler actions one by one to choose the one fits me most. The choice was super-labeler because it allows me what I want, see below liist:

* issue and pr labeling

* searching the files changes (label by package  label according to file change in  PR for monorepo)

* matching the creator of PR (label core-contributor issue/pr)

* matching branch name to label type of PR (feat, fix etc)

Also the cool thing in super-labeler is that I can store labels config in one JSON and the label will be created if not exist (it is powerful feature to have consistent labeling across few repos - my company maintain actively some OpenSource repos and we are aiming to be consisten in processes to manage and maintain then)

Before I started to use your repo I need to use 3 separate packages for that. Now I use only yours (I will post you side not e in separate issue because I have some problem with matching the files in PR).

This is pretty much the reason I made it, I found different actions that did the different things I wanted, but none of them did everything. I'm glad it's useful :)

I still have some use case that I would like to have and would like to raise some other PR in the future if you don't mind :)

Do you have any new feat that you would like to introduce in near future ? I will be glad to have also

* checking the PR size (count deletiion/addition similar to [codelytv/pr-size-labeler@v1
  action](https://github.com/CodelyTV/pr-size-labeler) )

* be able to pass `fallback` if the is no match for label when opening PR/issue (default `label` can be `need_triage` or something like that - my teammate @jayu came up with this idea when I show him your repo)

Thank you, those are good ideas, I'd like to implement those! Feel free to create an issue for each feature request (or let me know if you want me to do it). Some things I'd like to add:

Again, thank you for the contribution! Would you like your username to be linked in the next release notes?

jbinda commented 4 years ago

I'll update the README. Is this a breaking change? processRegExpPattern should detect if you're using the old style, right?

Until there will be proper backslashes of special characters it will work. For safety I will assume that it is breaking change. I was tested some cases manually using this code locally

I need to take a look at https://github.com/nektos/act at some point, I think it makes it possible to test the action locally, and maybe it could even be setup to run automated tests

I have also try to use this app but do not have much time to play with it - had some issue because my action fails when running through act. Probably will work more when have a chance

Again, thank you for the contribution! Would you like your username to be linked in the next release notes?

It's up to you - your repo your rules :) However I will be glad if you do some mention

jbinda commented 4 years ago

it's far from done, but if you want to see the progress so far, you can look at: https://ivanfon.github.io/super-labeler-action/

Nice work ! I think it will be very helpful. Specially if you also allow to add rules for matching as well (long term I suppose but worth to consider )