Back to Superset

Code Review Process

docs/developer_docs/contributing/code-review.md

2021.41.08.6 KB
Original Source
<!-- 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

External

Next: Reporting issues effectively