From 913ae1275d4bba0a0dec0bfc8b9a58275f4f193c Mon Sep 17 00:00:00 2001 From: M57 <41645758+Hy4ri@users.noreply.github.com> Date: Sun, 23 Aug 2026 17:32:32 +0300 Subject: [PATCH] docs(contributing): align contributor docs with real workflow + fix ktlint command (#960) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add "search for dupes first" rule (open/closed PRs + issues) before starting. - Fix ktlint instructions: the authoritative command is ./gradlew ktlintCheck / ktlintFormat, not the standalone ./ktlint binary (hand-fixes fail CI). - Document rebase-onto-main before squash-merge (stale-branch overwrite trap). - Add test-storage hygiene rule (no real on-device storage; TemporaryFolder / in-memory Room) — generic, not tied to any specific test harness or box. - Add dependency-pinning note (AGP/Kotlin/SDK "SDK not writable" trap). - Tighten PR template: type-of-change, rebased, ktlintCheck, navigateTo(). --- .github/PULL_REQUEST_TEMPLATE.md | 28 +++++++++++---- CONTRIBUTING.md | 59 +++++++++++++++++++++++++------- 2 files changed, 68 insertions(+), 19 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 7280c6cb..5b0e4a35 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -1,19 +1,35 @@ ## Summary - + Fixes # +## Type of Change + +- [ ] 🐛 Bug fix +- [ ] ✨ Feature +- [ ] ♻️ Refactor (no behavior change) +- [ ] 📝 Docs +- [ ] ✅ Tests +- [ ] 🔧 CI / chore + ## What changed - + ## How to test - + + +1. +2. +3. ## Checklist -- [ ] ktlint and CI pass -- [ ] Tested on a device or CI APK -- [ ] Navigation goes through `NavigationController.navigateTo()` +- [ ] Branch rebased onto `main` (`git rebase origin/main`) +- [ ] `./gradlew ktlintCheck` passes (ran `ktlintFormat` first) +- [ ] `./gradlew testDebugUnitTest` green (or CI unit-tests job) +- [ ] `checkColorLiterals` passes (no hardcoded Color outside theme/) +- [ ] Navigation goes through `NavigationController.navigateTo()` (not `backStack.add`) +- [ ] Tested on device/emulator or verified via CI APK diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7724270a..ba5ea1e7 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -4,18 +4,33 @@ Thank you for your interest in contributing to Hermes Mobile! To maintain high c --- +## Before You Start: Search First + +A quick duplicate check saves your time and keeps the PR queue clean: + +```bash +gh search prs --repo Hy4ri/hermes-mobile --state open "" +gh search prs --repo Hy4ri/hermes-mobile --state closed "" +gh search issues --repo Hy4ri/hermes-mobile --state open "" +``` + +If an open PR already covers it, review or improve that one instead of opening a competing duplicate. For larger work, comment on the issue to signal you're on it. + +--- + ## PR Workflow -All code changes must go through a pull request (PR). Directly pushing to `main` is not allowed. +All code changes must go through a pull request (PR). Directly pushing to `main` is not allowed **except for trivial changes the maintainer explicitly okays** — when in doubt, open a PR. 1. **Pick or open an issue** to discuss the changes you want to make. 2. **Create a branch** off `main` using the following naming convention: - - Features: `feat/issue-N-description` - - Bug fixes: `fix/issue-N-description` -3. **Implement your changes** and format them locally. -4. **Submit a PR** targeting the `main` branch. -5. **Ensure all CI checks pass** (formatting, linting, and unit tests). -6. **Maintainers will squash-merge** your PR into `main`. + - Features: `feat/short-description` (optionally `feat/issue-N-description`) + - Bug fixes: `fix/short-description` (optionally `fix/issue-N-description`) + - CI/chore: `ci/...`, `chore/...` +3. **Implement your changes** and format them locally (see Code Style). +4. **Submit a PR** targeting the `main` branch, filling the PR template. +5. **Ensure all CI checks pass** (ktlint, Android Lint, unit tests, build). +6. **Rebase onto `main` before merge.** Maintainers squash-merge; a stale branch's version of an unrelated file can silently overwrite recent fixes on `main` when squashed. `git fetch origin main && git rebase origin/main` first. --- @@ -57,34 +72,52 @@ If you use AI coding tools (including agents) to contribute: We enforce Kotlin coding conventions and Jetpack Compose best practices. ### Kotlin Formatting -- Code formatting is checked and enforced by **ktlint 1.2.1**. -- Run formatting check locally before committing: + +- Code formatting is checked and enforced by **ktlint 1.2.1** in CI. The authoritative command is the Gradle task (not the standalone binary): ```bash - ./ktlint # Check one file - ./ktlint --format # Auto-fix formatting issues + ./gradlew ktlintCheck # check (CI-equivalent) + ./gradlew ktlintFormat # auto-fix, then re-check with ktlintCheck ``` + > ⚠️ Hand-patching indentation/import order routinely fails the real CI gate (multiline-expression-wrapping and ASCII import order are the usual casualties). Always run `ktlintFormat`, never fix formatting by hand. - Import ordering is strictly ASCII-lexicographic (uppercase before lowercase: e.g., `LaunchedEffect` before `collectAsState`). ### Compose Guidelines + - Standard screen structures must use `HermesScaffold` rather than raw Material3 `Scaffold`. - Composable parameters must follow the standard order: `modifier` first, then event callbacks, and finally children content. - Do **not** apply `paddingValues` on inner content inside `HermesScaffold` — the scaffold already handles top bar padding. See `AGENTS.md` for the full breakdown of this recurring bug. - Every data screen must implement `LoadingState`, `ErrorState`, and `EmptyState` branches in its `when { }` block. ### Color Usage + - **No hardcoded `Color(0x...)` literals** outside `theme/`, `*Preview.kt`, and auth screens. A Gradle task (`checkColorLiterals`) enforces this in CI. - Use `MaterialTheme.colorScheme.` or `LocalHermesStatusColors.current.` instead. - `Color.Transparent` and `Color.Unspecified` are allowed. +### Tests + +- Unit tests: `./gradlew testDebugUnitTest` (MockK). Instrumented Compose UI tests run in CI on an emulator. +- **Tests must not touch real on-device storage.** Use `@get:Rule TemporaryFolder` (or Room `inMemoryDatabaseBuilder`) — never write to real app data or external storage. CI/emulator environments are ephemeral; leaking state breaks the next run. +- Mock time/dispatchers explicitly (`StandardTestDispatcher` + `Dispatchers.setMain`) — see `AGENTS.md` test references. + +--- + +## Dependency & Build Hygiene + +- **Pin build tooling.** AGP/Kotlin/SDK versions are pinned in `gradle/libs.versions.toml` and `build.gradle.kts` on purpose. Unpinned versions silently break the build on the next dependabot/CI refresh (the "SDK not writable" AGP trap). Bump deliberately, not via wildcard ranges. +- Never add a dependency without updating the version catalog; CI validates the Gradle wrapper. + --- ## PR Checklist Before submitting your PR, please verify: -- [ ] Your branch is up-to-date with `main`. -- [ ] Local build and `ktlint` checks pass successfully. +- [ ] I searched open/closed PRs + issues for duplicates (see *Before You Start*). +- [ ] Branch is rebased onto current `main` (`git rebase origin/main`). +- [ ] `./gradlew ktlintCheck` passes (ran `ktlintFormat` first — no hand edits). - [ ] `checkColorLiterals` passes (no hardcoded Color literals outside theme/). +- [ ] `./gradlew testDebugUnitTest` passes locally (or CI unit-tests job is green). - [ ] No unused imports, unused parameters, or dead code. - [ ] Every `Image` and `Icon` element has a descriptive `contentDescription` for accessibility. - [ ] New screens use `HermesScaffold` and implement Loading/Error/Empty states.