Paul Medynski
2 months ago
committed by
GitHub
GitHub
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 330 additions and 118 deletions
-
34.github/PULL_REQUEST_TEMPLATE/pr-template.md
-
2CONTRIBUTING.md
-
22README.md
-
102policy/coding-best-practices.md
-
0policy/coding-style.md
-
74policy/review-process.md
@ -0,0 +1,34 @@ |
|||||
|
## Description |
||||
|
|
||||
|
Provide a summary of the changes being introduced. Important topics to cover |
||||
|
include: |
||||
|
|
||||
|
- Description of the functionality. |
||||
|
- API changes, backwards compatibility, deprecations, etc. |
||||
|
- Documentation, localization. |
||||
|
- Bug fixes. |
||||
|
- Code hygiene, refactoring, improvements. |
||||
|
- Engineering processes (CI, pipelines, test coverage) |
||||
|
|
||||
|
High quality descriptions will lead to a smoother review experience. |
||||
|
|
||||
|
## Issues |
||||
|
|
||||
|
Link to any relevant issues, bugs, or discussions (e.g., "Closes \#123", "Fixes |
||||
|
issue \#456"). |
||||
|
|
||||
|
## Testing |
||||
|
|
||||
|
Describe the automated tests (unit, integration) you created or modified. |
||||
|
Provide justification for any gap in automated testing. List any manual testing |
||||
|
steps that were performed to ensure the changes work. |
||||
|
|
||||
|
## Guidelines |
||||
|
|
||||
|
Please review the contribution guidelines before submitting a pull request: |
||||
|
|
||||
|
- [Contributing](/CONTRIBUTING.md) |
||||
|
- [Code of Conduct](/CODE_OF_CONDUCT.md) |
||||
|
- [Best Practices](/policy/coding-best-practices.md) |
||||
|
- [Coding Style](/policy/coding-style.md) |
||||
|
- [Review Process](/policy/review-process.md) |
||||
@ -0,0 +1,102 @@ |
|||||
|
# Coding Best Practices |
||||
|
|
||||
|
This document describes some typical programming pitfalls and best practices |
||||
|
related to C# and .NET. It will grow and change as we encounter new situations |
||||
|
and the codebase evolves. |
||||
|
|
||||
|
## Correctness & Business Logic |
||||
|
|
||||
|
- Validate if the code adheres to what it is supposed to do. Compare the |
||||
|
implementation with the specification and ask questions, to get clarity. |
||||
|
- Employ `Debug.Assert()` statements to validate program state, but never |
||||
|
rely on them to control program flow. For example, `Debug.Assert()` must |
||||
|
never be used to validate input. |
||||
|
|
||||
|
## Memory / Resource Management |
||||
|
|
||||
|
- C# – Rent large buffers from ArrayPool<T> and return them promptly to |
||||
|
cut down on heap allocations and improve throughput. Have the buffers been |
||||
|
returned to the pool? Will they be returned if an exception is thrown? |
||||
|
- Prefer Span<T> for stack allocated buffers. |
||||
|
- Double check the depth of the call stack if using async/await. Stack depth of |
||||
|
unwinding async/await state-machines of more than 3 calls can cause |
||||
|
performance issues. |
||||
|
- Resource cleanup Dispose(), Close(), or the equivalent to release files, |
||||
|
sockets, database connections, and other external resources. |
||||
|
|
||||
|
## Error Handling & Reliability |
||||
|
|
||||
|
- Prefer return values to throwing exceptions. Returns are strongly typed and |
||||
|
checked by the compiler. Consider patterns like |
||||
|
`bool TryXYZ(..., out Result result)` instead of `Result XYZ(...)`. |
||||
|
- Exceptions should be reserved for exceptional circumstances, like constructor |
||||
|
failure or out-of-memory, where the calling code stands little chance of |
||||
|
recovering. Routine, expected error conditions should not be expressed as |
||||
|
exceptions. |
||||
|
- Document all exceptions that may be thrown from an API. |
||||
|
- Do not let undocumented exceptions escape from an API. |
||||
|
- Be permissive with what you accept, and strict with what you produce. |
||||
|
- Consider Constrained Execution Regions for .NET Framework implementations. |
||||
|
|
||||
|
## Async, Concurrency & Thread Safety (if applicable) |
||||
|
|
||||
|
- Avoid calling Task.Result, Task.Wait() or Task.GetAwaiter().GetResult(). If |
||||
|
needed, exercise extreme caution. These can cause deadlocks. |
||||
|
- When awaiting a task with the await keyword, always use |
||||
|
Task.ConfigureAwait(false) so that the task continuation may run on any thread |
||||
|
rather than marshalling back to the original calling thread. |
||||
|
|
||||
|
## Backward Compatibility |
||||
|
|
||||
|
- Someone will always depend on your undocumented behaviour. It is just as much |
||||
|
an API promise as explicitly documented behaviour. |
||||
|
- Use language features to deprecate/obsolete APIs, and to issue warnings for |
||||
|
notable upcoming breaking changes. |
||||
|
|
||||
|
## Security Considerations |
||||
|
|
||||
|
- Never log passwords, secrets, or connection strings with credentials. |
||||
|
- Validate inputs to avoid SQL injection, even on metadata calls. |
||||
|
- Are there any user inputs going into SQL or shell commands directly? |
||||
|
- Are secrets being logged or exposed in stack traces? |
||||
|
- Are TLS/certificate settings handled safely and explicitly? |
||||
|
- Are we sending unbounded data streams to server prior to authentication e.g. |
||||
|
in feature extensions? |
||||
|
|
||||
|
## Performance & Scalability |
||||
|
|
||||
|
- For major features or large PRs, always run the internal performance |
||||
|
benchmarks or performance testsuite to determine if the new changes are |
||||
|
causing any performance degradation. |
||||
|
|
||||
|
## Observability (Logging / Tracing / Metrics) |
||||
|
|
||||
|
- Error messages should contain enough context to understand the state of the |
||||
|
program and find the source code responsible for emitting it. |
||||
|
- Instrument changes for metrics/tracing/logging to aid future field diagnosis, |
||||
|
debugging, and app behaviour analysis. |
||||
|
|
||||
|
## Unit Tests / Integration |
||||
|
|
||||
|
- Have you added unit tests and integration tests? |
||||
|
- Unit tests should not depend on external resources. Code under unit test |
||||
|
should permit mocked resources to be provided/injected. |
||||
|
- Avoid tests that rely on timing. |
||||
|
|
||||
|
## Configuration & Feature Flags |
||||
|
|
||||
|
- Is the change considered a breaking change? If breaking change is not |
||||
|
avoidable, has a Configuration/Feature flag been introduced for customers to |
||||
|
revert to old behavior? |
||||
|
|
||||
|
## Code Coverage expectations |
||||
|
|
||||
|
- Ideally these expectations should be codified into scanning tool |
||||
|
configuration that lives in the repo. |
||||
|
- Specific metrics will be chosen based on realistic analysis of existing |
||||
|
coverage and will be re-evaluated as testing matures. |
||||
|
|
||||
|
## Pipeline runs |
||||
|
|
||||
|
- Is the CI passing? If not, then have you looked at the failures, and found the |
||||
|
cause for failures? |
||||
@ -0,0 +1,74 @@ |
|||||
|
# Review Process |
||||
|
|
||||
|
This document describes guidelines and responsibilities for participants of |
||||
|
pull request reviews. |
||||
|
|
||||
|
## Author Responsibilities |
||||
|
|
||||
|
- **Describe the problem up front.** In the PR description, include a concise, |
||||
|
self-contained summary of the issue being solved and link to relevant specs, |
||||
|
issues, or discussions. For open-source PRs, capture the context in one clear |
||||
|
section so reviewers don’t have to sift through comment threads. |
||||
|
- **Ship code with tests.** Submit unit and integration tests alongside the |
||||
|
implementation to demonstrate expected behavior and prevent regressions. |
||||
|
- **Instrument for production.** Add meaningful tracing or logging so the |
||||
|
changes can be monitored and debugged once deployed. |
||||
|
- **Keep PRs focused.** Split large or mixed-purpose changes into smaller, |
||||
|
logical units (e.g., separate refactors from feature work). |
||||
|
- **Highlight important changes.** Add your own comments to the diffs to call |
||||
|
out important changes that require extra scrutiny, or to explain a change |
||||
|
whose inclusion isn't self-evident. |
||||
|
- **Choose your reviewers.** The author should assign an initial set of |
||||
|
required and optional reviewers. Reviewers may change their role later after |
||||
|
discussing with the author. |
||||
|
- **Any open issue blocks completion.** Do not complete a review with open |
||||
|
issues. All issues must be resolved by the reviewer who created them, or |
||||
|
another reviewer delegate. Do not override tooling that prevents completion. |
||||
|
- **Do not resolve reviewer issues.** Only reviewers should resolve reviewer |
||||
|
issues. Ideally the creator of the issue should resolve it, but reviewers may |
||||
|
close each other's issues under certain circumstances. |
||||
|
|
||||
|
## Reviewer Responsibilities |
||||
|
|
||||
|
- **Understand what you’re approving.** If anything is unclear, ask questions. |
||||
|
Require in-code comments that explain why a solution exists - not just what it |
||||
|
does. PR comments are not a substitute for code comments. |
||||
|
- **Read the full PR description.** Context matters; don’t skip it. |
||||
|
- **Review the changes, not the design.** A code review is not the time to |
||||
|
review a design choice. If you think an alternative design would be better, |
||||
|
discuss with the author and team, and ask for a new PR if the new design is |
||||
|
implemented instead. |
||||
|
- **Demand tests.** If appropriate tests are missing, request them. When |
||||
|
testing truly isn’t feasible, insist on a documented rationale. |
||||
|
- **Note partial reviews.** If you reviewed only part of the code, say so in |
||||
|
the PR. |
||||
|
- **Delegate wisely.** If you cannot complete your reviewer role's |
||||
|
responsibilities, delegate to another subject-matter expert. |
||||
|
- **Own your approval.** You are accountable for the quality and correctness of |
||||
|
the code you sign off on. |
||||
|
- **Block completion on all open issues.** Any issue you open against a review |
||||
|
must be resolved to your satisfaction before a review may complete. |
||||
|
Resolution may include new code changes to fix a problem, new in-code |
||||
|
documentation to explain something, or a discussion within the PR itself. |
||||
|
- **Do not close other reviewer's issues.** The reviewer who created an issue |
||||
|
should be the one to resolve it. Exceptions include explicit delegation or |
||||
|
OOTO absences. Any delegations should be discussed with the team. |
||||
|
- **Never rubber-stamp.** Trust the author but verify the code - always conduct |
||||
|
a real review. |
||||
|
- **Review in a timely manner.** Reviewers should aim to perform a review |
||||
|
within 2 business days of receiving the initial request, and 1 business day |
||||
|
for follow-up changes. Reviewing a PR is higher priority than most other |
||||
|
tasks. |
||||
|
|
||||
|
## Backwards Compatibility Awareness |
||||
|
|
||||
|
Subtle changes may have backwards compatibility implications. Below are some of |
||||
|
the code changes and side-effects to watch out for: |
||||
|
|
||||
|
- Do these changes force updates to existing integration tests - a sign they may |
||||
|
introduce a breaking change? |
||||
|
- Will these changes modify the public API’s behavior in a way that could break |
||||
|
existing applications when they upgrade to the new binaries? |
||||
|
- For any non-security, breaking changes that impact customers, have we provided |
||||
|
an opt-out - such as a connection-string flag, AppContext switch, or similar |
||||
|
setting - that lets users revert to the previous behavior? |
||||
Write
Preview
Loading…
Cancel
Save
Reference in new issue