mirror of
https://github.com/apache/superset.git
synced 2026-04-07 18:35:15 +00:00
docs: bifurcate documentation into user and admin sections (#38196)
Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
Evan Rusackas
339
docs/developer_docs/contributing/code-review.md
Normal file
339
docs/developer_docs/contributing/code-review.md
Normal file
@@ -0,0 +1,339 @@
|
||||
---
|
||||
title: Code Review Process
|
||||
sidebar_position: 4
|
||||
---
|
||||
|
||||
<!--
|
||||
Licensed to the Apache Software Foundation (ASF) under one
|
||||
or more contributor license agreements. See the NOTICE file
|
||||
distributed with this work for additional information
|
||||
regarding copyright ownership. The ASF licenses this file
|
||||
to you under the Apache License, Version 2.0 (the
|
||||
"License"); you may not use this file except in compliance
|
||||
with the License. You may obtain a copy of the License at
|
||||
|
||||
http://www.apache.org/licenses/LICENSE-2.0
|
||||
|
||||
Unless required by applicable law or agreed to in writing,
|
||||
software distributed under the License is distributed on an
|
||||
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
|
||||
KIND, either express or implied. See the License for the
|
||||
specific language governing permissions and limitations
|
||||
under the License.
|
||||
-->
|
||||
|
||||
# Code Review Process
|
||||
|
||||
Understand how code reviews work in Apache Superset and how to participate effectively.
|
||||
|
||||
## Overview
|
||||
|
||||
Code review is a critical part of maintaining code quality and sharing knowledge across the team. Every change to Superset goes through peer review before merging.
|
||||
|
||||
## For Authors
|
||||
|
||||
### Preparing for Review
|
||||
|
||||
#### Before Requesting Review
|
||||
- [ ] Self-review your changes
|
||||
- [ ] Ensure CI checks pass
|
||||
- [ ] Add comprehensive tests
|
||||
- [ ] Update documentation
|
||||
- [ ] Fill out PR template completely
|
||||
- [ ] Add screenshots for UI changes
|
||||
|
||||
#### Self-Review Checklist
|
||||
```bash
|
||||
# View your changes
|
||||
git diff upstream/master
|
||||
|
||||
# Check for common issues:
|
||||
# - Commented out code
|
||||
# - Debug statements (console.log, print)
|
||||
# - TODO comments that should be addressed
|
||||
# - Hardcoded values that should be configurable
|
||||
# - Missing error handling
|
||||
# - Performance implications
|
||||
```
|
||||
|
||||
### Requesting Review
|
||||
|
||||
#### Auto-Assignment
|
||||
GitHub will automatically request reviews based on CODEOWNERS file.
|
||||
|
||||
#### Manual Assignment
|
||||
For specific expertise, request additional reviewers:
|
||||
- Frontend changes: Tag frontend experts
|
||||
- Backend changes: Tag backend experts
|
||||
- Security changes: Tag security team
|
||||
- Database changes: Tag database experts
|
||||
|
||||
#### Review Request Message
|
||||
```markdown
|
||||
@reviewer This PR implements [feature]. Could you please review:
|
||||
1. The approach taken in [file]
|
||||
2. Performance implications of [change]
|
||||
3. Security considerations for [feature]
|
||||
|
||||
Thanks!
|
||||
```
|
||||
|
||||
### Responding to Feedback
|
||||
|
||||
#### Best Practices
|
||||
- **Be receptive**: Reviews improve code quality
|
||||
- **Ask questions**: Clarify if feedback is unclear
|
||||
- **Explain decisions**: Share context for your choices
|
||||
- **Update promptly**: Address feedback in timely manner
|
||||
|
||||
#### Comment Responses
|
||||
```markdown
|
||||
# Acknowledging
|
||||
"Good catch! Fixed in [commit hash]"
|
||||
|
||||
# Explaining
|
||||
"I chose this approach because [reason]. Would you prefer [alternative]?"
|
||||
|
||||
# Questioning
|
||||
"Could you elaborate on [concern]? I'm not sure I understand the issue."
|
||||
|
||||
# Disagreeing respectfully
|
||||
"I see your point, but I think [current approach] because [reason]. What do you think?"
|
||||
```
|
||||
|
||||
## For Reviewers
|
||||
|
||||
### Review Responsibilities
|
||||
|
||||
#### What to Review
|
||||
1. **Correctness**: Does the code do what it claims?
|
||||
2. **Design**: Is the approach appropriate?
|
||||
3. **Clarity**: Is the code readable and maintainable?
|
||||
4. **Testing**: Are tests comprehensive?
|
||||
5. **Performance**: Any performance concerns?
|
||||
6. **Security**: Any security issues?
|
||||
7. **Documentation**: Is it well documented?
|
||||
|
||||
### Review Checklist
|
||||
|
||||
#### Functionality
|
||||
- [ ] Feature works as described
|
||||
- [ ] Edge cases are handled
|
||||
- [ ] Error handling is appropriate
|
||||
- [ ] Backwards compatibility maintained
|
||||
|
||||
#### Code Quality
|
||||
- [ ] Follows project conventions
|
||||
- [ ] No code duplication
|
||||
- [ ] Clear variable/function names
|
||||
- [ ] Appropriate abstraction levels
|
||||
- [ ] SOLID principles followed
|
||||
|
||||
#### Testing
|
||||
- [ ] Unit tests for business logic
|
||||
- [ ] Integration tests for APIs
|
||||
- [ ] E2E tests for critical paths
|
||||
- [ ] Tests are maintainable
|
||||
- [ ] Good test coverage
|
||||
|
||||
#### Security
|
||||
- [ ] Input validation
|
||||
- [ ] SQL injection prevention
|
||||
- [ ] XSS prevention
|
||||
- [ ] CSRF protection
|
||||
- [ ] Authentication/authorization checks
|
||||
- [ ] No sensitive data in logs
|
||||
|
||||
#### Performance
|
||||
- [ ] Database queries optimized
|
||||
- [ ] No N+1 queries
|
||||
- [ ] Appropriate caching
|
||||
- [ ] Frontend bundle size impact
|
||||
- [ ] Memory usage considerations
|
||||
|
||||
### Providing Feedback
|
||||
|
||||
#### Effective Comments
|
||||
|
||||
```python
|
||||
# ✅ Good: Specific and actionable
|
||||
"This query could cause N+1 problems. Consider using
|
||||
`select_related('user')` to fetch users in a single query."
|
||||
|
||||
# ❌ Bad: Vague
|
||||
"This doesn't look right."
|
||||
```
|
||||
|
||||
```typescript
|
||||
// ✅ Good: Suggests improvement
|
||||
"Consider using useMemo here to prevent unnecessary
|
||||
re-renders when dependencies haven't changed."
|
||||
|
||||
// ❌ Bad: Just criticism
|
||||
"This is inefficient."
|
||||
```
|
||||
|
||||
#### Comment Types
|
||||
|
||||
**Use GitHub's comment types:**
|
||||
- **Comment**: General feedback or questions
|
||||
- **Approve**: Changes look good
|
||||
- **Request Changes**: Must be addressed before merge
|
||||
|
||||
**Prefix conventions:**
|
||||
- `nit:` Minor issue (non-blocking)
|
||||
- `suggestion:` Recommended improvement
|
||||
- `question:` Seeking clarification
|
||||
- `blocker:` Must be fixed
|
||||
- `praise:` Highlighting good work
|
||||
|
||||
#### Examples
|
||||
|
||||
```markdown
|
||||
nit: Consider renaming `getData` to `fetchUserData` for clarity
|
||||
|
||||
suggestion: This could be simplified using Array.reduce()
|
||||
|
||||
question: Is this intentionally not handling the error case?
|
||||
|
||||
blocker: This SQL is vulnerable to injection. Please use parameterized queries.
|
||||
|
||||
praise: Excellent test coverage! 👍
|
||||
```
|
||||
|
||||
## Review Process
|
||||
|
||||
### Timeline
|
||||
|
||||
#### Expected Response Times
|
||||
- **Initial review**: Within 2-3 business days
|
||||
- **Follow-up review**: Within 1-2 business days
|
||||
- **Critical fixes**: ASAP (tag in Slack)
|
||||
|
||||
#### Escalation
|
||||
If no response after 3 days:
|
||||
1. Ping reviewer in PR comments
|
||||
2. Ask in #development Slack channel
|
||||
3. Tag @apache/superset-committers
|
||||
|
||||
### Approval Requirements
|
||||
|
||||
#### Minimum Requirements
|
||||
- **1 approval** from a committer for minor changes
|
||||
- **2 approvals** for significant features
|
||||
- **3 approvals** for breaking changes
|
||||
|
||||
#### Special Cases
|
||||
- **Security changes**: Require security team review
|
||||
- **API changes**: Require API team review
|
||||
- **Database migrations**: Require database expert review
|
||||
- **UI/UX changes**: Require design review
|
||||
|
||||
### Merge Process
|
||||
|
||||
#### Who Can Merge
|
||||
- Committers with write access
|
||||
- After all requirements met
|
||||
- CI checks must pass
|
||||
|
||||
#### Merge Methods
|
||||
- **Squash and merge**: Default for feature PRs
|
||||
- **Rebase and merge**: For clean history
|
||||
- **Create merge commit**: Rarely used
|
||||
|
||||
#### Merge Checklist
|
||||
- [ ] All CI checks green
|
||||
- [ ] Required approvals obtained
|
||||
- [ ] No unresolved conversations
|
||||
- [ ] PR title follows conventions
|
||||
- [ ] Milestone set (if applicable)
|
||||
|
||||
## Review Etiquette
|
||||
|
||||
### Do's
|
||||
- ✅ Be kind and constructive
|
||||
- ✅ Acknowledge time and effort
|
||||
- ✅ Provide specific examples
|
||||
- ✅ Suggest solutions
|
||||
- ✅ Praise good work
|
||||
- ✅ Consider cultural differences
|
||||
- ✅ Focus on the code, not the person
|
||||
|
||||
### Don'ts
|
||||
- ❌ Use harsh or dismissive language
|
||||
- ❌ Bikeshed on minor preferences
|
||||
- ❌ Review when tired or frustrated
|
||||
- ❌ Make personal attacks
|
||||
- ❌ Ignore the PR description
|
||||
- ❌ Demand perfection
|
||||
|
||||
## Becoming a Reviewer
|
||||
|
||||
### Path to Reviewer
|
||||
1. **Contribute regularly**: Submit quality PRs
|
||||
2. **Participate in discussions**: Share knowledge
|
||||
3. **Review others' code**: Start with comments
|
||||
4. **Build expertise**: Focus on specific areas
|
||||
5. **Get nominated**: By existing committers
|
||||
|
||||
### Reviewer Expectations
|
||||
- Review PRs in your area of expertise
|
||||
- Respond within reasonable time
|
||||
- Mentor new contributors
|
||||
- Maintain high standards
|
||||
- Stay current with best practices
|
||||
|
||||
## Advanced Topics
|
||||
|
||||
### Reviewing Large PRs
|
||||
|
||||
#### Strategy
|
||||
1. **Request splitting**: Ask to break into smaller PRs
|
||||
2. **Review in phases**:
|
||||
- Architecture/approach first
|
||||
- Implementation details second
|
||||
- Tests and docs last
|
||||
3. **Use draft reviews**: Save comments and submit together
|
||||
|
||||
### Cross-Team Reviews
|
||||
|
||||
#### When Needed
|
||||
- Changes affecting multiple teams
|
||||
- Shared components/libraries
|
||||
- API contract changes
|
||||
- Database schema changes
|
||||
|
||||
### Performance Reviews
|
||||
|
||||
#### Tools
|
||||
```python
|
||||
# Backend performance
|
||||
import cProfile
|
||||
import pstats
|
||||
|
||||
# Profile the code
|
||||
cProfile.run('function_to_profile()', 'stats.prof')
|
||||
stats = pstats.Stats('stats.prof')
|
||||
stats.sort_stats('cumulative').print_stats(10)
|
||||
```
|
||||
|
||||
```typescript
|
||||
// Frontend performance
|
||||
// Use React DevTools Profiler
|
||||
// Chrome DevTools Performance tab
|
||||
// Lighthouse audits
|
||||
```
|
||||
|
||||
## Resources
|
||||
|
||||
### Internal
|
||||
- [Coding Guidelines](../guidelines/design-guidelines)
|
||||
- [Testing Guide](../testing/overview)
|
||||
- [Extension Architecture](../extensions/architecture)
|
||||
|
||||
### External
|
||||
- [Google's Code Review Guide](https://google.github.io/eng-practices/review/)
|
||||
- [Best Practices for Code Review](https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/)
|
||||
- [The Art of Readable Code](https://www.oreilly.com/library/view/the-art-of/9781449318482/)
|
||||
|
||||
Next: [Reporting issues effectively](./issue-reporting)
|
||||
Reference in New Issue
Block a user