-
Notifications
You must be signed in to change notification settings - Fork 13
docs: add pull request review checklist #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
Merged
osm-vishnukyatannawar
merged 7 commits into
OsmosysSoftware:main
from
Jatin-1602:feat/PR-review
Mar 5, 2025
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
e0f4329
Add PR review checklist
Jatin-1602 df5c05d
Rename the file
Jatin-1602 f73cb46
Update the checklist
Jatin-1602 5aa22de
Remove optional mark for dependencies
Jatin-1602 dad5b7d
Add logging details
Jatin-1602 eb8a620
Update the dependices section
Jatin-1602 debace1
Merge branch 'main' into feat/PR-review
osm-vishnukyatannawar File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| # Standard PR Review Checklist | ||
|
|
||
| ## PR Meta Information | ||
|
|
||
| - **PR Title**: Ensure the PR title is clear, concise, and adheres to the project's naming standards (e.g., `[Feature] Add X`, `[Bugfix] Fix Y`, `[Refactor] Z`). | ||
| - **Description**: Include a detailed description explaining: | ||
| - What the PR does. | ||
| - Why the changes are necessary. | ||
| - Links to related issues, tickets, or tasks (if applicable). | ||
| - **Checklist in Description (Optional)**: Include a checklist of tasks completed (if the project uses task checklists). | ||
| - **Reviewer Assignment**: The PR must be assigned to at least one reviewer. | ||
| - **Labels/Tags (Optional)**: Ensure the PR has appropriate labels/tags (e.g., `Bugfix`, `Feature`, `Documentation`). | ||
| - **Branch Naming**: Follow the project's branch naming conventions (e.g., `feature/xyz`, `bugfix/abc`). | ||
|
|
||
| ## Code Changes | ||
|
|
||
| - **Scope of Changes**: | ||
| - PR should contain changes related to only one feature, bugfix, or task. | ||
| - Avoid mixing multiple unrelated changes in one PR. | ||
| - PRs should not exceed a maximum of 500 lines of code (excluding tests and documentation). | ||
| - **Clean Code**: | ||
| - Code should follow the project's coding standards (naming conventions, indentation, spacing). | ||
| - Ensure proper formatting (auto-format code if tools are available). | ||
| - **No Repetition**: Avoid duplicate code; refactor into reusable functions or components where possible. | ||
| - **No Hardcoded Values**: Replace hardcoded values with constants, environment variables, or configurations. | ||
| - **Breaking Down Large Changes**: | ||
| - For large features, break down changes into smaller, manageable PRs. | ||
|
|
||
| ## Commit Messages | ||
|
|
||
| - **Format**: Ensure commit messages follow the [company's git standards](https://github.com/OsmosysSoftware/dev-standards/blob/main/coding-standards/git.md). | ||
| - Example: `[Component] Short description of change` | ||
| - Use imperative mood (e.g., "Fix bug" instead of "Fixed bug"). | ||
| - Provide detailed explanations if the change is complex. | ||
|
|
||
| ## Quality and Best Practices | ||
|
|
||
| - **Tests**: | ||
| - Ensure unit tests or integration tests are added/updated for the feature or bugfix. | ||
| - Run all tests and confirm they pass locally. | ||
| - Include both positive and negative test cases. | ||
| - **Logging**: | ||
| - Follow project's logging standards. | ||
| - Include appropriate log levels (DEBUG, INFO, ERROR). | ||
| - Avoid sensitive information in logs. | ||
| - **Linting and Static Analysis**: | ||
| - Code should pass all linting checks (e.g., ESLint, Flake8, etc.). | ||
| - Resolve any warnings or errors flagged by static analysis tools. | ||
| - **Error Handling**: Ensure proper error handling is implemented (avoid swallowing errors silently). | ||
| - **Performance Considerations**: Check for code efficiency (e.g., avoid unnecessary loops, database queries, or API calls). | ||
|
|
||
| ## Compliance | ||
|
|
||
| - **CI/CD Pipelines**: | ||
| - Ensure all CI pipelines (builds, tests, deployments) pass successfully. | ||
| - Attach a CI screenshot if the pipelines aren't integrated into the PR system. | ||
| - **Security**: | ||
| - Verify no sensitive information (API keys, passwords, etc.) is exposed in the code or configuration. | ||
| - Check for usage of secure methods and libraries. | ||
| - **Backward Compatibility**: Ensure the changes don't break existing functionality or introduce regressions. | ||
|
|
||
| ## Documentation and UI | ||
|
|
||
| - **Documentation**: Update relevant documentation (e.g., README, API documentation, comments) for new features or changes. | ||
| - **Screenshots/Videos (Optional)**: | ||
| - Add screenshots or video recordings for UI changes (if applicable). | ||
| - Ensure the UI matches the design and is responsive on different screen sizes. | ||
|
|
||
| ## Miscellaneous | ||
|
|
||
| - **Dead Code**: Remove any unused code, variables, or imports. | ||
| - **Debugging Artifacts**: Ensure no debugging code (e.g., `console.log`, `print`) is left in the codebase. | ||
| - **Dependencies**: | ||
| - Verify no unnecessary or outdated dependencies are introduced. | ||
| - Lock file changes (e.g., `package-lock.json`, `requirements.txt`) should be reviewed. | ||
| - Check for security vulnerabilities in new dependencies. | ||
| - Ensure compatibility with existing dependencies. | ||
|
|
||
| ## Final Checks | ||
|
|
||
| - **Self-Review**: The developer submitting the PR must perform a thorough self-review before assigning it to a reviewer. | ||
| - **Cross-Environment Testing (Optional)**: Confirm the changes work in multiple environments (e.g., dev, staging, production). | ||
| - **Feature Flags (Optional)**: Ensure new features are behind feature flags if they are not fully ready for release. | ||
osm-vishnukyatannawar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.