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

CodeRabbit config #35636

Merged
merged 5 commits into from
Jan 20, 2025
Merged

CodeRabbit config #35636

merged 5 commits into from
Jan 20, 2025

Conversation

Charl1996
Copy link
Contributor

@Charl1996 Charl1996 commented Jan 20, 2025

Technical Summary

This PR merely adds a couple of configs to the recently-added CodeRabbit tool, like telling it to not do an auto-review (and only when prompted).

The CodeRabbit-HQ integration is experimental and will be tested during the next 2 or so weeks. As such the config might change as time goes on and new needs are being discovered.

Rollback instructions

Rolling back this PR will roll back the config which means CodeRabbit might act differently

@Charl1996 Charl1996 requested a review from millerdev January 20, 2025 08:01
.coderabbit.yaml Outdated
Comment on lines 14 to 16
- path: "https://github.com/dimagi/open-source/blob/master/docs/Writing_PRs.md"
instructions: |
Please follow this guide when evaluating the pull request and make sure that the PR meets all the requirements.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

path_instructions are to give additional instructions for how to review files that match the path regex. Adding this file here won't change how it reviews other code, only this one file.

You may be able to add this kind of info via the Learnings database: https://docs.coderabbit.ai/integrations/knowledge-base

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've wondered about that.

So you're suggesting that we rather configure CodeRabbit on the fly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you're suggesting that we rather configure CodeRabbit on the fly?

I'm not what you mean by 'on the fly'. Do you mean without any special instructions? We can add instructions but will need to copy them into the config and use regex to have them applied to specific sets of files. I'm not sure if the ones you had for JS files would work (whether it would load the info from the link) but I suspect not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@snopoke I want the assistant to following the guidelines we have in HQ in the docs as context to evaluating any PR, so my question was around if we should/could only tell the assistant to use the said guidelines docs when speaking to it during a review process as opposed to configuring it in the coderabbit.yaml file?

I guess it's OK to only tell it during a review process (I know it can learn from one PR and apply to future PRs), but then we lose visibility into what the assistant considers for context when reviewing a PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this path instructions config be used to ignore file types? Reviewing diff files like this isn't helpful. Reviewing dependency management files is also questionable - this review is noise.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found it - there's a path_filters config.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so my question was around if we should/could only tell the assistant to use the said guidelines docs when speaking to it during a review process as opposed to configuring it in the coderabbit.yaml file?

The only way I know of to do this would be to pass it in the yaml config file as you were except you'd need to include the full text and not just links to the file that has the info. Also you have to use a regex to tell it what file paths the instructions apply to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also you have to use a regex to tell it what file paths the instructions apply to

Helpful to know! Missed that somehow in the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orangejenny Thanks for the feedback! Will update!

high_level_summary: false
review_status: false
poem: false
collapse_walkthrough: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this instead be turned off and only included when requested? It adds a lot of clutter to emails.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good suggestion. Will update.

@Charl1996
Copy link
Contributor Author

I'll merge this for now and respond to the feedback in a followup PR.

@Charl1996 Charl1996 merged commit 3d84ee6 into master Jan 20, 2025
13 checks passed
@Charl1996 Charl1996 deleted the ai-reviewer branch January 20, 2025 13:33
auto_reply: true
enabled: false
auto_incremental_review: false
drafts: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we want auto-review on drafts yet. Should we turn it off until we are confident that we want automatic reviews?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

.coderabbit.yaml Outdated
# Code Review Instructions

- Ensure the code follows best practices and coding standards.
- For **Python** code, follow
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why you did not replace this with links to our code review guidelines, most of which were provided in this Slack thread?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh...good question. I moved it to the path_instructions, but let me add it back here and see what it does. I'm not sure if it will read the links, but let's test!

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

Successfully merging this pull request may close these issues.

4 participants