-
Notifications
You must be signed in to change notification settings - Fork 76
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
Add copilot-instructions.md #4561
base: main
Are you sure you want to change the base?
Conversation
.github/copilot-instructions.md
Outdated
- Methods shouldn't be too big, or do too much on their own. If a method is too big, maybe it should be multiple methods instead. | ||
- Use compiler supported immutability in places where we don't need to change objects (records, readonly properties...) | ||
- Never throw a general Exception (throw new Exception) | ||
- We always use structural logging in our code |
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.
This sounds like it can be easily misinterpreted, for example a piece of code that doesn't log anything (which is often totally fine) would violate this rule. Maybe it should be more something like "When adding logs, prioritize structured logging over unstructured log messages"
.github/copilot-instructions.md
Outdated
- Use compiler supported immutability in places where we don't need to change objects (records, readonly properties...) | ||
- Never throw a general Exception (throw new Exception) | ||
- We always use structural logging in our code | ||
- Each method should log what it does, not the caller |
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.
Same point as the comment above, this sounds dangerously close to saying that all methods should log what they're doing.
Maybe this doesn't need to be a rule. I was frustrated with the amount of noise that's added in the code of PullRequestUpdater because of all the logging, but we've discussed it many times and the consensus was that that's how we want to do it. If we really followed this rule we wouldn't log anything in the main methods of PullRequestUpdater
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 think it might be fine, as it's easy to just delete the line that does the logging? Better to have it than not I think
- For multi-task methods, use descriptive names and document the process. | ||
- Long method names are acceptable. | ||
- Async methods must have an "Async" suffix. | ||
- Avoid using booleans solely to signal success. |
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.
- Avoid using booleans solely to signal success. | |
- Avoid using booleans solely to signal success, unless failure is an expected frequent outcome (e.g. TryGet methods). |
- Validate early; if validation fails, throw or return immediately. | ||
- After validation, use if/else—not returns—for control flow. | ||
- Keep methods focused; split them if too large. | ||
- Use compiler-supported immutability (e.g., records, readonly properties) when possible. |
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.
- Use compiler-supported immutability (e.g., records, readonly properties) when possible. | |
- Use immutable objects (e.g., records, readonly properties) when possible. |
#4560