RFR: 755: Anything starting with a "/" is treated as a command

Erik Joelsson erikj at openjdk.org
Fri Feb 24 15:52:55 UTC 2023


On Fri, 24 Feb 2023 15:43:42 GMT, Vijay Kulkarni <duke at openjdk.org> wrote:

> This seem like too simplistic an approach to me, in that it trades one problem for another. Now a typo in a command name, such as `/intigrate` or similar, will show up as a review comment without the helpful "unrecognized command" comment and suggestion to type `/help`.

I think this change is good and I directed Vijay to tackle this issue. 
 
It's a difficult tradeoff for sure, but I've seen the command extractor trigger on unintentional command looking comments more often than I would like. Especially the filtering when sending emails is problematic. See for example this PR and corresponding email:

https://github.com/openjdk/jdk/pull/12695
https://mail.openjdk.org/pipermail/build-dev/2023-February/037829.html

I'm not sure why the PR bot didn't reply to that instance though.

> I might recommend the more targeted solution of not treating `/sometext` as a command when it appears in a code block.

That would be another approach, but it would require a more complicated parsing logic. I could also imagine adding some kind of fuzzy matcher on the existing commands so that obvious spelling errors were caught. I'm not sure it's worth the effort.

> As for other problematic cases, wasn't there an earlier change to not treat `/sometext` as a command unless it was at the beginning of a line (after trimming whitespace)? If not, then that would also be a good change.
> 
> I'll test that theory now: /integrate which Skara should ignore.

It's correct that we only try to match commands preceded by whitespace. I just don't think it's enough to avoid enough false positive matches.

-------------

PR: https://git.openjdk.org/skara/pull/1479


More information about the skara-dev mailing list