nuttx icon indicating copy to clipboard operation
nuttx copied to clipboard

[Feedback] PR Review Bot

Open lupyuen opened this issue 1 year ago • 6 comments

Please provide your feedback for the NuttX PR Review Bot. Thanks!

lupyuen avatar Sep 20 '24 06:09 lupyuen

My comment on this message from bot:

Squash The Commits: This PR contains 2 Commits. Please Squash the Multiple Commits into a Single Commit.

A suggestion like this shouldn't be based solely on the number of commits in PR. What should be squashed and what shouldn't require a greater understanding of PR content. Having multiple commits in one PR isn't always bad. Sometimes it's even necessary.

raiden00pl avatar Sep 20 '24 12:09 raiden00pl

A suggestion like this shouldn't be based solely on the number of commits in PR. What should be squashed and what shouldn't require a greater understanding of PR content.

Sorry @cederom and @acassis: What are your thoughts about this? Shall I remove the "Squash Commit" message, since our bot can't analyse the PR Content? Thanks!

lupyuen avatar Sep 20 '24 12:09 lupyuen

maybe we can just change "Please squash commits" to "consider squashing commits if that makes sense for this PR" (or something similar)

raiden00pl avatar Sep 20 '24 12:09 raiden00pl

OK I removed the check for Squash Commits, I don't we're ready to handle it now :-)

lupyuen avatar Sep 20 '24 13:09 lupyuen

Thank you @lupyuen I think it is better not to include it to avoid inducing contributors to wrong direction.

It could be something that reviewers could do better

acassis avatar Sep 20 '24 13:09 acassis

Yes we need increase PR and git commit descriptions quality (both need to be self explanatory).. bot was created to help.. you can .. wait was there this nice copy-paste section where all sections were filled in already before

@cederom Yeah that's the problem with LLMs, we can't force it to say the same thing all the time :-) It seems to have a mind of its own / multiple personas?

nuttxpr avatar Sep 23 '24 11:09 nuttxpr

PR Text:

## Testing
...
Target(s): arch(sim), sim:mnemofs.

Bot reply:

Target Specificity: "sim:mnemofs" is good, but be even more specific if possible (e.g., which simulator, exact config used). More detail makes it easier for reviewers to reproduce your results.

This might be due to less fine tuning for the sim cases?

resyfer avatar Oct 22 '24 07:10 resyfer

A suggestion to add a way to trigger the bot again. For example...if it gave some feedback, and I improved it, I don't have any way of checking it again without creating another PR (which would mean wasted GH checks as PR text and build runs are often unrelated).

resyfer avatar Oct 22 '24 07:10 resyfer