-
-
Notifications
You must be signed in to change notification settings - Fork 218
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 updates #35645
Coderabbit/config updates #35645
Conversation
.coderabbit.yaml
Outdated
- path: "**/*.py" | ||
instructions: >- | ||
- Review the code following best practises and standards | ||
path_filters: ["**/*.js", "**/*.py", "**/*.md", "**/*.rst"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with making this opt-in. I think you could add HTML files here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTML! Good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
.coderabbit.yaml
Outdated
path_instructions: | ||
- path: "**/*.js" | ||
instructions: >- | ||
- Review the JavaScript code against the Google JavaScript style guide and point out any mismatches |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a link to "Google JavaScript style guide"?
@@ -13,7 +13,7 @@ reviews: | |||
path_instructions: | |||
- path: "**/*.js" | |||
instructions: >- | |||
- Review the JavaScript code against the Google JavaScript style guide and point out any mismatches | |||
- Review the JavaScript code against the [Google JavaScript style guide](https://google.github.io/styleguide/jsguide.html) and point out any mismatches |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a note on the site saying
Please note: This guide is no longer being updated. Google recommends migrating to TypeScript, and following the TypeScript guide.
Do we rather want to include that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not use TypeScript, so that doesn't seem right. I think this is fine for now. We may want to investigate other options for JS guides that are maintained.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Figured as much. The "Google JavaScript style guide" I got from examples of what other people online use and I figured that it sounds OK, but definitely open to investigate this further.
@@ -13,7 +13,7 @@ reviews: | |||
path_instructions: | |||
- path: "**/*.js" | |||
instructions: >- | |||
- Review the JavaScript code against the Google JavaScript style guide and point out any mismatches | |||
- Review the JavaScript code against the [Google JavaScript style guide](https://google.github.io/styleguide/jsguide.html) and point out any mismatches |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not use TypeScript, so that doesn't seem right. I think this is fine for now. We may want to investigate other options for JS guides that are maintained.
Makes some updates to the coderabbit configuration file.
I'm not 100% sure if we can simply give it links like I did in the instructions and assume it will load the content as context, but that's what I want to test out.
I'm also trying to tell it to only review certain files, as seen in the
path_filters
config. I thought it's probably best to tell it what we want it to review rather than what we don't want to reduce unintentional noise. Maybe we can flip it.