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