Initial commit
This commit is contained in:
Zhongwei Li
552
assets/bad-pr-example.md
Normal file
552
assets/bad-pr-example.md
Normal file
@@ -0,0 +1,552 @@
|
||||
# Bad PR Example
|
||||
|
||||
This example demonstrates common mistakes that annoy maintainers and lead to PR rejection or delays. Learn what NOT to do!
|
||||
|
||||
---
|
||||
|
||||
## Example: Adding OAuth2 Authentication (Done Wrong)
|
||||
|
||||
### PR Title ❌
|
||||
```
|
||||
Updated code
|
||||
```
|
||||
|
||||
**Why it's bad:**
|
||||
- ❌ Too vague
|
||||
- ❌ Doesn't describe what changed
|
||||
- ❌ No type prefix (feat/fix/docs)
|
||||
- ❌ Wrong tense (past instead of imperative)
|
||||
- ❌ Lowercase (should capitalize)
|
||||
|
||||
**Should be:**
|
||||
```
|
||||
feat(auth): add OAuth2 support for Google and GitHub providers
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### PR Description ❌
|
||||
|
||||
```markdown
|
||||
## Changes
|
||||
Added OAuth
|
||||
|
||||
## Testing
|
||||
Works for me
|
||||
```
|
||||
|
||||
**Why it's bad:**
|
||||
- ❌ No explanation of WHAT was added
|
||||
- ❌ No explanation of WHY it's needed
|
||||
- ❌ No explanation of HOW it works
|
||||
- ❌ No testing instructions for reviewers
|
||||
- ❌ No issue links
|
||||
- ❌ No screenshots
|
||||
- ❌ "Works for me" is not helpful
|
||||
|
||||
**Result:** Maintainer has to ask multiple questions before even starting review.
|
||||
|
||||
---
|
||||
|
||||
### Files Changed (Messy!) ❌
|
||||
|
||||
```
|
||||
.env # ❌ SECRETS COMMITTED!
|
||||
.vscode/settings.json # ❌ Personal IDE config
|
||||
SESSION.md # ❌ Personal development notes
|
||||
planning/oauth-implementation.md # ❌ Planning documents
|
||||
screenshots/debug-oauth-1.png # ❌ Debug screenshots
|
||||
screenshots/debug-oauth-2.png # ❌ Debug screenshots
|
||||
screenshots/test-local.png # ❌ Personal testing screenshots
|
||||
test-manual.js # ❌ Temporary test file
|
||||
NOTES.md # ❌ Personal notes
|
||||
TODO.md # ❌ Personal todo list
|
||||
node_modules/ # ❌ Dependencies (should be in .gitignore!)
|
||||
package-lock.json # ❌ Lock file changes (not needed)
|
||||
src/routes/auth.js # ✅ Actual feature code
|
||||
src/routes/users.js # ❌ Unrelated changes
|
||||
src/routes/posts.js # ❌ Unrelated changes
|
||||
src/components/Header.js # ❌ Unrelated changes
|
||||
src/utils/formatting.js # ❌ Unrelated refactoring
|
||||
README.md # ✅ Documentation update
|
||||
docs/authentication.md # ✅ OAuth documentation
|
||||
```
|
||||
|
||||
**Problems:**
|
||||
- ❌ **28 files changed** (way too many!)
|
||||
- ❌ **Secrets committed** (.env file with API keys)
|
||||
- ❌ **Personal artifacts** (SESSION.md, NOTES.md, planning/)
|
||||
- ❌ **Unrelated changes** (users.js, posts.js, Header.js)
|
||||
- ❌ **Mixed concerns** (feature + refactoring + bug fixes)
|
||||
|
||||
**Should be:** 5-8 files, only those directly related to OAuth feature.
|
||||
|
||||
---
|
||||
|
||||
### Commit History (Terrible!) ❌
|
||||
|
||||
```
|
||||
WIP
|
||||
fixed stuff
|
||||
asdf
|
||||
more changes
|
||||
fix
|
||||
actually working now
|
||||
Final commit
|
||||
Actually final
|
||||
OK this one is final
|
||||
oops forgot something
|
||||
```
|
||||
|
||||
**Why it's bad:**
|
||||
- ❌ 10 commits for one feature (should squash)
|
||||
- ❌ Meaningless commit messages
|
||||
- ❌ "WIP" commits (not production-ready)
|
||||
- ❌ No explanation of what changed
|
||||
- ❌ No issue links
|
||||
- ❌ Commit history shows trial-and-error (should clean up)
|
||||
|
||||
**Should be:** 1-2 clean commits with proper messages.
|
||||
|
||||
---
|
||||
|
||||
### SESSION.md Content (Should Never Be Committed!) ❌
|
||||
|
||||
```markdown
|
||||
# OAuth Implementation Session
|
||||
|
||||
## 2025-11-04
|
||||
Started working on OAuth. Not sure if I should use passport or just implement it myself. Going to try passport first.
|
||||
|
||||
## 2025-11-05
|
||||
Passport is confusing. Spent 3 hours debugging. Finally got it working but the code is messy.
|
||||
|
||||
TODO:
|
||||
- Clean up the callback logic
|
||||
- Add tests (maybe)
|
||||
- Figure out how to handle errors
|
||||
- Ask @maintainer about rate limiting?
|
||||
|
||||
NOTES:
|
||||
- Google OAuth app ID: 123456789-abcdefgh.apps.googleusercontent.com
|
||||
- Redirect URI: http://localhost:3000/auth/google/callback
|
||||
- Remember to add to staging: https://staging.myapp.com/auth/google/callback
|
||||
|
||||
BUGS FOUND:
|
||||
- Header component has alignment issue on mobile
|
||||
- Post creation form doesn't validate correctly
|
||||
- User profile page crashes when avatar is null
|
||||
```
|
||||
|
||||
**Why this should NEVER be in a PR:**
|
||||
- ❌ Personal development journal
|
||||
- ❌ Reveals your confusion/struggle
|
||||
- ❌ Contains unfinished TODOs
|
||||
- ❌ Mentions unrelated bugs
|
||||
- ❌ Unprofessional appearance
|
||||
- ❌ Pollutes the project
|
||||
|
||||
---
|
||||
|
||||
### NOTES.md Content (Should Never Be Committed!) ❌
|
||||
|
||||
```markdown
|
||||
# Development Notes
|
||||
|
||||
## OAuth Research
|
||||
Looked at how GitHub and GitLab do OAuth. Their implementations are pretty complex. Mine is simpler.
|
||||
|
||||
## Design Decisions
|
||||
- Using passport because it's easier
|
||||
- Not implementing token refresh yet (can do later)
|
||||
- Rate limiting - should probably add this but skipping for now
|
||||
- Testing - added some tests but not complete coverage
|
||||
|
||||
## Things I'm Not Sure About
|
||||
- Is the error handling good enough?
|
||||
- Should I use sessions or JWT?
|
||||
- Do I need to validate the email from OAuth providers?
|
||||
|
||||
## Known Issues
|
||||
- Doesn't work in Safari (CORS issue)
|
||||
- Memory leak in callback handler (need to fix)
|
||||
- Missing rate limiting (security risk?)
|
||||
```
|
||||
|
||||
**Why this hurts your PR:**
|
||||
- ❌ Shows incomplete work
|
||||
- ❌ Admits to known issues not mentioned in PR
|
||||
- ❌ Reveals security concerns not addressed
|
||||
- ❌ Makes maintainer lose confidence
|
||||
- ❌ Creates more work for maintainer
|
||||
|
||||
---
|
||||
|
||||
### Communication During Review ❌
|
||||
|
||||
**Initial Comment:**
|
||||
```
|
||||
Here's my OAuth implementation. Let me know what you think.
|
||||
```
|
||||
|
||||
**Why it's bad:**
|
||||
- ❌ No context
|
||||
- ❌ No explanation
|
||||
- ❌ No testing instructions
|
||||
- ❌ Sounds careless
|
||||
|
||||
---
|
||||
|
||||
**Response to Feedback (Poor):**
|
||||
|
||||
**Maintainer:** "Could you add tests for the error cases?"
|
||||
|
||||
**Bad Response:**
|
||||
```
|
||||
Tests are boring. The code works fine without them.
|
||||
```
|
||||
|
||||
**Why it's bad:**
|
||||
- ❌ Dismissive
|
||||
- ❌ Unprofessional
|
||||
- ❌ Doesn't follow project standards
|
||||
- ❌ Shows lack of respect
|
||||
|
||||
---
|
||||
|
||||
**Maintainer:** "This PR is too large. Could you split it into smaller PRs?"
|
||||
|
||||
**Bad Response:**
|
||||
```
|
||||
It's all related though. I don't want to spend time splitting it up.
|
||||
```
|
||||
|
||||
**Why it's bad:**
|
||||
- ❌ Refuses reasonable request
|
||||
- ❌ Doesn't consider reviewer's time
|
||||
- ❌ Makes review harder
|
||||
- ❌ Likely to be closed
|
||||
|
||||
---
|
||||
|
||||
**No Response for 2 Weeks** ❌
|
||||
|
||||
**Maintainer:** "The tests are failing. Can you fix them?"
|
||||
|
||||
**Your response:** [Silence for 2 weeks]
|
||||
|
||||
**Why it's bad:**
|
||||
- ❌ Wastes maintainer's time
|
||||
- ❌ PR goes stale
|
||||
- ❌ Likely to be closed
|
||||
- ❌ Damages your reputation
|
||||
|
||||
---
|
||||
|
||||
### PR Metrics ❌
|
||||
|
||||
**Size:**
|
||||
- Lines changed: 847
|
||||
- Files changed: 28
|
||||
- Commits: 10
|
||||
|
||||
**Problems:**
|
||||
- ❌ Way too large (should be <200 lines)
|
||||
- ❌ Too many files (includes unrelated changes)
|
||||
- ❌ Messy commit history
|
||||
|
||||
**Timeline:**
|
||||
- Submitted: Day 1
|
||||
- Maintainer requests changes: Day 2
|
||||
- No response: Days 3-16
|
||||
- PR closed as stale: Day 17
|
||||
|
||||
---
|
||||
|
||||
## Specific Mistakes Breakdown
|
||||
|
||||
### 1. Committed Secrets ❌
|
||||
|
||||
**.env file contents:**
|
||||
```
|
||||
GOOGLE_CLIENT_ID=123456789-abcdefgh.apps.googleusercontent.com
|
||||
GOOGLE_CLIENT_SECRET=GOCSPX-abc123def456 # ❌ SECRET!
|
||||
GITHUB_CLIENT_ID=Iv1.1234567890abcdef
|
||||
GITHUB_CLIENT_SECRET=1234567890abcdef1234567890abcdef12345678 # ❌ SECRET!
|
||||
DATABASE_URL=postgresql://admin:SuperSecret123@localhost/myapp # ❌ PASSWORD!
|
||||
```
|
||||
|
||||
**Impact:**
|
||||
- 🚨 Security breach!
|
||||
- 🚨 Must rotate all secrets immediately
|
||||
- 🚨 Potentially compromises production
|
||||
- 🚨 Even if removed later, it's in git history
|
||||
|
||||
**What you should have done:**
|
||||
- ✅ Only commit .env.example with placeholder values
|
||||
- ✅ Add .env to .gitignore
|
||||
- ✅ Never commit actual secrets
|
||||
|
||||
---
|
||||
|
||||
### 2. Including Unrelated Changes ❌
|
||||
|
||||
**src/routes/users.js:**
|
||||
```javascript
|
||||
// OAuth PR includes this "drive-by fix"
|
||||
exports.getUser = async (req, res) => {
|
||||
const user = await User.findById(req.params.id);
|
||||
// Fixed bug where avatar URL was broken
|
||||
if (user.avatar) {
|
||||
user.avatar = user.avatar.replace('http://', 'https://');
|
||||
}
|
||||
res.json(user);
|
||||
};
|
||||
```
|
||||
|
||||
**Why it's bad:**
|
||||
- ❌ Not related to OAuth feature
|
||||
- ❌ Makes PR harder to review
|
||||
- ❌ Mixed concerns
|
||||
- ❌ If PR is reverted, this fix goes too
|
||||
|
||||
**What you should have done:**
|
||||
- ✅ Create separate PR for bug fix
|
||||
- ✅ Keep OAuth PR focused on OAuth only
|
||||
|
||||
---
|
||||
|
||||
### 3. Massive PR with No Breakdown ❌
|
||||
|
||||
**Includes all at once:**
|
||||
- OAuth implementation (200 lines)
|
||||
- Refactoring of existing auth (300 lines)
|
||||
- Bug fixes in unrelated files (150 lines)
|
||||
- UI updates (100 lines)
|
||||
- Test updates (97 lines)
|
||||
|
||||
**Total: 847 lines in 28 files**
|
||||
|
||||
**Why it's bad:**
|
||||
- ❌ Takes hours to review
|
||||
- ❌ Hard to find bugs
|
||||
- ❌ Difficult to discuss
|
||||
- ❌ Can't merge incrementally
|
||||
|
||||
**What you should have done:**
|
||||
- ✅ PR #1: Refactor existing auth (300 lines)
|
||||
- ✅ PR #2: Add OAuth backend (200 lines)
|
||||
- ✅ PR #3: Add OAuth UI (100 lines)
|
||||
- ✅ PR #4: Fix related bugs (150 lines)
|
||||
|
||||
---
|
||||
|
||||
### 4. Poor Testing ❌
|
||||
|
||||
**test-manual.js (Committed by mistake!):**
|
||||
```javascript
|
||||
// Quick test script - DELETE BEFORE COMMIT!
|
||||
const axios = require('axios');
|
||||
|
||||
async function testOAuth() {
|
||||
// This only works on my machine
|
||||
const response = await axios.get('http://localhost:3000/auth/google');
|
||||
console.log('Works!');
|
||||
}
|
||||
|
||||
testOAuth();
|
||||
```
|
||||
|
||||
**Why it's bad:**
|
||||
- ❌ Not a proper test
|
||||
- ❌ Hardcoded localhost
|
||||
- ❌ No assertions
|
||||
- ❌ Comment says "DELETE BEFORE COMMIT"
|
||||
- ❌ Clutters project
|
||||
|
||||
**What you should have done:**
|
||||
- ✅ Delete this file
|
||||
- ✅ Write proper tests in tests/auth/oauth.test.js
|
||||
- ✅ Use project's testing framework
|
||||
- ✅ Include assertions and edge cases
|
||||
|
||||
---
|
||||
|
||||
### 5. Missing Documentation ❌
|
||||
|
||||
**README.md changes:**
|
||||
```markdown
|
||||
## Authentication
|
||||
|
||||
We now have OAuth.
|
||||
```
|
||||
|
||||
**Why it's bad:**
|
||||
- ❌ No setup instructions
|
||||
- ❌ No explanation of how it works
|
||||
- ❌ No examples
|
||||
- ❌ Unhelpful to users
|
||||
|
||||
**Should be:**
|
||||
```markdown
|
||||
## Authentication
|
||||
|
||||
### OAuth 2.0 Social Login
|
||||
|
||||
Users can sign in with Google or GitHub accounts.
|
||||
|
||||
#### Setup
|
||||
|
||||
1. Create OAuth apps:
|
||||
- Google: https://console.cloud.google.com/apis/credentials
|
||||
- GitHub: https://github.com/settings/developers
|
||||
|
||||
2. Add credentials to `.env`:
|
||||
```
|
||||
GOOGLE_CLIENT_ID=your_client_id
|
||||
GOOGLE_CLIENT_SECRET=your_client_secret
|
||||
GITHUB_CLIENT_ID=your_client_id
|
||||
GITHUB_CLIENT_SECRET=your_client_secret
|
||||
```
|
||||
|
||||
3. Restart server
|
||||
|
||||
#### Usage
|
||||
|
||||
Users will see "Sign in with Google" and "Sign in with GitHub" buttons on the login page.
|
||||
|
||||
For detailed implementation, see [docs/authentication.md](docs/authentication.md).
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 6. Ignoring CI Failures ❌
|
||||
|
||||
**CI Status:**
|
||||
```
|
||||
❌ Tests: 5 failing
|
||||
❌ Lint: 23 errors
|
||||
❌ Build: Failed
|
||||
```
|
||||
|
||||
**Your response:** Submit PR anyway, hope maintainer doesn't notice
|
||||
|
||||
**Why it's bad:**
|
||||
- ❌ Shows you didn't test
|
||||
- ❌ Wastes CI resources
|
||||
- ❌ Can't merge with failing CI
|
||||
- ❌ Unprofessional
|
||||
|
||||
**What you should have done:**
|
||||
- ✅ Fix all CI issues before submitting
|
||||
- ✅ Run tests locally first: `npm test`
|
||||
- ✅ Run lint locally: `npm run lint`
|
||||
- ✅ Only submit when all checks pass
|
||||
|
||||
---
|
||||
|
||||
## The Result
|
||||
|
||||
**Maintainer's Response:**
|
||||
```
|
||||
Thanks for the PR, but there are several issues:
|
||||
|
||||
1. You've committed secrets in the .env file - this is a security issue
|
||||
2. The PR includes unrelated changes (users.js, posts.js, Header.js)
|
||||
3. Personal development files (SESSION.md, NOTES.md, planning/) shouldn't be here
|
||||
4. The PR is too large - 847 lines across 28 files
|
||||
5. Tests are failing
|
||||
6. Missing proper documentation
|
||||
7. test-manual.js is committed
|
||||
|
||||
Please:
|
||||
- Remove all secrets and rotate them
|
||||
- Create separate PRs for unrelated changes
|
||||
- Remove personal development artifacts
|
||||
- Fix the failing tests
|
||||
- Add proper documentation
|
||||
- Keep the PR focused on OAuth only
|
||||
|
||||
I'm closing this for now. Please feel free to submit a new PR addressing these issues.
|
||||
```
|
||||
|
||||
**Status:** ❌ PR Closed
|
||||
|
||||
---
|
||||
|
||||
## Key Lessons
|
||||
|
||||
### What Went Wrong
|
||||
|
||||
1. **Security**: Committed secrets (.env file)
|
||||
2. **Scope**: Too large, too many unrelated changes
|
||||
3. **Artifacts**: Personal files committed (SESSION.md, NOTES.md)
|
||||
4. **Testing**: Poor testing, CI failures
|
||||
5. **Documentation**: Inadequate documentation
|
||||
6. **Communication**: Poor responses to feedback
|
||||
7. **Quality**: Messy commits, no code review
|
||||
8. **Professionalism**: Dismissive attitude
|
||||
|
||||
### How to Fix It
|
||||
|
||||
1. **Security**
|
||||
- Never commit secrets
|
||||
- Use .env.example with placeholders
|
||||
- Run pre-PR check script
|
||||
|
||||
2. **Scope**
|
||||
- Keep PRs small (<200 lines)
|
||||
- One feature per PR
|
||||
- No unrelated changes
|
||||
|
||||
3. **Artifacts**
|
||||
- Remove SESSION.md, NOTES.md, TODO.md
|
||||
- Remove planning/ directory
|
||||
- Remove debug screenshots
|
||||
- Use clean-branch script
|
||||
|
||||
4. **Testing**
|
||||
- Write proper tests
|
||||
- Fix CI before submitting
|
||||
- Test locally first
|
||||
|
||||
5. **Documentation**
|
||||
- Update README
|
||||
- Add setup instructions
|
||||
- Include examples
|
||||
|
||||
6. **Communication**
|
||||
- Be responsive
|
||||
- Be respectful
|
||||
- Accept feedback gracefully
|
||||
|
||||
7. **Quality**
|
||||
- Clean commit history
|
||||
- Proper commit messages
|
||||
- Review your own code
|
||||
|
||||
8. **Professionalism**
|
||||
- Respect maintainer's time
|
||||
- Follow project conventions
|
||||
- Be patient and courteous
|
||||
|
||||
---
|
||||
|
||||
## Comparison: Bad vs Good
|
||||
|
||||
| Aspect | Bad PR ❌ | Good PR ✅ |
|
||||
|--------|-----------|-----------|
|
||||
| **Title** | "Updated code" | "feat(auth): add OAuth2 support" |
|
||||
| **Size** | 847 lines, 28 files | 180 lines, 9 files |
|
||||
| **Commits** | 10 messy commits | 1 clean commit |
|
||||
| **Files** | Includes SESSION.md, .env | Only relevant files |
|
||||
| **Testing** | test-manual.js, CI failing | Proper tests, CI passing |
|
||||
| **Docs** | "We now have OAuth" | Complete setup guide |
|
||||
| **Secrets** | Committed .env | Only .env.example |
|
||||
| **Scope** | OAuth + bugs + refactor | OAuth only |
|
||||
| **Communication** | Dismissive | Professional |
|
||||
| **Result** | Closed | Merged in 3 days |
|
||||
|
||||
---
|
||||
|
||||
**Remember:** Every mistake in this example is based on real PRs that have been rejected. Learn from these errors and follow the good PR example instead!
|
||||
Reference in New Issue
Block a user