Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feedback] PR Review Bot #13552

Closed
lupyuen opened this issue Sep 20, 2024 · 8 comments
Closed

[Feedback] PR Review Bot #13552

lupyuen opened this issue Sep 20, 2024 · 8 comments

Comments

@lupyuen
Copy link
Member

lupyuen commented Sep 20, 2024

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

@raiden00pl
Copy link
Contributor

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.

@lupyuen
Copy link
Member Author

lupyuen commented Sep 20, 2024

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!

@raiden00pl
Copy link
Contributor

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

@lupyuen
Copy link
Member Author

lupyuen commented Sep 20, 2024

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

@acassis
Copy link
Contributor

acassis commented Sep 20, 2024

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

@nuttxpr
Copy link

nuttxpr commented Sep 23, 2024

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?

@lupyuen lupyuen closed this as completed Oct 4, 2024
@resyfer
Copy link
Contributor

resyfer commented Oct 22, 2024

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
Copy link
Contributor

resyfer commented Oct 22, 2024

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants