Allow other options to enter pr #55
Conversation
|
I could have made the regex patterns a single pattern, but it became extremely hard to read so I kept them seperate |
src/commands/ptal.ts
Outdated
| pathSections.shift(); | ||
| if(githubURL.hostname != "github.com" || pathSections.length != 4 || pathSections[2] != "pull" || !Number.parseInt(pathSections[3])) | ||
| const githubRE = /https:\/\/github\.com\/(?<ORGANISATION>[^\/]+)\/(?<REPOSITORY>[^\/]+)\/pull\/(?<NUMBER>\d+)/; | ||
| const otherRE = /((?<ORGANISATION>[^\/]+)\/)?(?<REPOSITORY>[^(#|\s)]+)(#|\s)(?<NUMBER>\d+)/; |
There was a problem hiding this comment.
In the second regex, it may be worth to also match /<org?>/<repo?>/pull/<number> as well, because currently the options are either a full URL or /<org?>/<repo?>#<number>
The latter doesn't actually resolve in github (except #<number> inline, but that only works for a single repo)
There was a problem hiding this comment.
Also /org/repo/ 123 is valid because of (#|\s), I don't think that was intentional but feel free to correct me if I'm wrong
There was a problem hiding this comment.
I had made both "https://" and "https://github.com/" optional so that should work now. I have removed the optional whitespace between the number and the repo. No clue why I added that
| repo, | ||
| pull_number | ||
| owner: groups["ORGANISATION"] ?? "withastro", | ||
| repo: groups["REPOSITORY"], |
There was a problem hiding this comment.
No default repository, but it is optional in otherRE
(Is it meant to be optional in otherRE? I would have expected for #123 to resolve to withastro/astro/pulls/123 but maybe that was not the intention?)
There was a problem hiding this comment.
I don’t believe it’s optional in otherRE
There was a problem hiding this comment.
I see, / 123 and similar matched the regex so I thought maybe it was meant to be, but looking again that is most likely not intentional.
There was a problem hiding this comment.
In that case it could be quite handy to add a list of allowed "syntaxes" to the bot message
There was a problem hiding this comment.
I see,
/ 123and similar matched the regex so I thought maybe it was meant to be, but looking again that is most likely not intentional.
Yeah. I’m guessing that it matches the repo name as “/“ in that case
There was a problem hiding this comment.
My comment on the other review also addresses this issue
|
With those few regex tweaks otherwise LGTM |
Parsing now supports:
https://github.com/withastro/houston-bot/pull/52withastro/houston-bot#52houston-bot#52Also resolves a crash when an invalid deployment or other URL is provided