Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions .github/PULL_REQUEST_TEMPLATE.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,19 @@
## Related Issue

<!--
Required: every PR must relate to an issue. The "PR Issue Link" check fails
until this description links one. Write one line per issue:

Closes #123 this PR fully resolves the issue (Fixes/Resolves also work)
Refs #123 related, but the issue stays open
Part of #123 one step of a larger issue that stays open
Related to #123 same as Refs

Only a closing keyword directly before the number closes the issue on merge,
and each issue needs its own keyword ("Closes #1, closes #2", not
"Closes #1, #2"). A number in the title does nothing.
-->

## Summary

<!-- What does this PR do and why? -->
Expand Down
4 changes: 3 additions & 1 deletion .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -521,7 +521,7 @@ jobs:
- name: Run Python guard tests with coverage
run: |
mkdir -p coverage
guards='scripts/check_proguard_serial_keep.py,scripts/check_apk_signing_cert.py,scripts/check_native_libs_present.py,scripts/check_bundled_native_assets.py,scripts/check_jni_local_refs.py,scripts/check_dc_process_isolation.py,scripts/check_ci_success_gate.py,scripts/fix_macho_symbol_order.py,scripts/release/sanitize_apple_store_notes.py'
guards='scripts/check_proguard_serial_keep.py,scripts/check_apk_signing_cert.py,scripts/check_native_libs_present.py,scripts/check_bundled_native_assets.py,scripts/check_jni_local_refs.py,scripts/check_dc_process_isolation.py,scripts/check_ci_success_gate.py,scripts/check_pr_issue_link.py,scripts/fix_macho_symbol_order.py,scripts/release/sanitize_apple_store_notes.py'
python3 -m coverage run --include="$guards" \
scripts/check_proguard_serial_keep_test.py
python3 -m coverage run --append --include="$guards" \
Expand All @@ -536,6 +536,8 @@ jobs:
scripts/check_dc_process_isolation_test.py
python3 -m coverage run --append --include="$guards" \
scripts/check_ci_success_gate_test.py
python3 -m coverage run --append --include="$guards" \
scripts/check_pr_issue_link_test.py
python3 -m coverage run --append --include="$guards" \
scripts/fix_macho_symbol_order_test.py
python3 -m coverage run --append --include="$guards" \
Expand Down
77 changes: 77 additions & 0 deletions .github/workflows/pr-issue-link.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
name: PR Issue Link

# Every pull request must link the issue it relates to (issue #1800): a
# closing keyword (`Closes #N`) when it resolves the issue, or `Refs #N` /
# `Part of #N` / `Related to #N` when it does not. Without the keyword GitHub
# leaves a resolved issue open after the merge. The rule itself lives in
# scripts/check_pr_issue_link.py, which Script Tests in ci.yaml unit-tests.
#
# Kept out of ci.yaml on purpose. The check has to re-run when the description
# is edited, and adding `edited` to ci.yaml's triggers would restart the whole
# pipeline on every description edit. It blocks a merge by being listed as a
# required status check ("PR Issue Link") next to CI Success in main's branch
# protection. No `paths` filter: a required check that never reports leaves
# the PR waiting forever.
#
# pull_request_target, not pull_request: under pull_request the workflow and
# the script come from the PR's merge ref, so a PR could edit either to report
# green and walk through its own gate. pull_request_target runs both as they
# exist on main, and its check run still lands on the PR's head commit, where
# branch protection looks for it. That trigger is safe here only because this
# job never checks out or executes PR code: the checkout below is main, and
# the description is read through the API as data. Keep it that way. Two
# consequences: a PR that changes the rule is judged by main's copy until it
# merges (Script Tests still runs its own unit tests), and the PR that first
# adds this file gets no run.

on:
pull_request_target:
branches: [main]
types: [opened, edited, reopened, synchronize]

permissions:
contents: read
pull-requests: read

concurrency:
group: ${{ github.workflow }}-${{ github.event.pull_request.number }}
cancel-in-progress: true

jobs:
issue-link:
name: PR Issue Link
runs-on: ubuntu-latest
timeout-minutes: 5
steps:
# No `ref:`: under pull_request_target the default is main. Never point
# this at the PR head; that would run untrusted code with this token.
- uses: actions/checkout@v7
with:
sparse-checkout: scripts
persist-credentials: false

- name: Fetch the current PR description
# Read live rather than from the event payload: re-running a failed
# job replays the original payload, which would still hold the
# description from before the author fixed it.
env:
GH_TOKEN: ${{ github.token }}
REPO: ${{ github.repository }}
PR_NUMBER: ${{ github.event.pull_request.number }}
run: |
gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.body // ""' \
> "${RUNNER_TEMP}/pr_body.md"

- name: Check the description links an issue
# Untrusted PR fields reach the script only through env vars, never
# through ${{ }} interpolation inside the shell text.
env:
REPO: ${{ github.repository }}
HEAD_REF: ${{ github.event.pull_request.head.ref }}
AUTHOR_TYPE: ${{ github.event.pull_request.user.type }}
run: |
python3 scripts/check_pr_issue_link.py \
--body-file "${RUNNER_TEMP}/pr_body.md" \
--branch "$HEAD_REF" \
--author-type "$AUTHOR_TYPE" \
--repo "$REPO"
21 changes: 21 additions & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,27 @@ git at them.

**Bypass (if needed):** `git push --no-verify`

## Pull Requests

Every PR must relate to an issue, and its description must say which one. The
"PR Issue Link" check (`scripts/check_pr_issue_link.py`) blocks the merge until
it does.

- **Resolves the issue:** `Closes #123` (or `Fixes` / `Resolves`). GitHub
closes the issue on merge only when the keyword sits directly before the
number in the PR description. A number in the title, or a passing mention in
prose, closes nothing, which is how resolved issues were left open (#1800).
- **Relates without resolving it** (one phase of a larger issue, a follow-up):
`Refs #123`, `Part of #123` or `Related to #123`. The issue stays open.
- **Several issues:** one keyword per issue, `Closes #1, closes #2`.
`Closes #1, #2` closes only #1.
- **Branch names an issue** (`github-issue-1800-...`, `feature-request-1803-...`):
the description must link that issue, with `Refs` if the PR does not resolve it.
- **No issue yet:** open one first. Only bot-authored PRs are exempt.

Links inside HTML comments, code spans or fenced code blocks are ignored, by
GitHub and by the check. Editing the description re-runs the check.

## Gotchas

- The `dives` table uses `diveDateTime` (not `dateTime`) as the column name to
Expand Down
16 changes: 13 additions & 3 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,20 @@ files you touched) before committing.

1. Push your branch and open a pull request against `main`.
2. Fill out the [pull request template](.github/PULL_REQUEST_TEMPLATE.md),
describing what changed and why, and link any related issues.
3. Keep PRs focused and reasonably small — one logical change per PR is easier
describing what changed and why.
3. **Link the issue the PR relates to.** Every PR must relate to an issue, and
the "PR Issue Link" check blocks the merge until the description links one:
- `Closes #123` (or `Fixes` / `Resolves`) when the PR fully resolves the
issue. GitHub closes the issue on merge only when the keyword sits
directly before the number in the description; a number in the title does
nothing. Give each issue its own keyword: `Closes #1, closes #2`.
- `Refs #123`, `Part of #123` or `Related to #123` when the PR relates to an
issue without resolving it, so the issue stays open.

If no issue exists yet, open one first.
4. Keep PRs focused and reasonably small: one logical change per PR is easier
to review and merge.
4. Ensure CI passes. Maintainers may request changes; discussion is part of the
5. Ensure CI passes. Maintainers may request changes; discussion is part of the
process.

For detailed conventions, see:
Expand Down
110 changes: 74 additions & 36 deletions docs/contributing/pull-requests.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ This guide explains how to submit effective pull requests.
2. Check [open PRs](https://github.com/submersion-app/submersion/pulls)
3. Review the [roadmap](contributing/roadmap.md)

Every PR must relate to an issue (see [Linking Issues](#linking-issues)). If
none exists for your change, open one before you open the PR.

### Discuss Large Changes

For significant changes:
Expand All @@ -32,7 +35,8 @@ git remote add upstream https://github.com/submersion-app/submersion.git

# Create branch
git checkout -b feature/your-feature
```text
```

### 2. Make Changes

- Follow [code style](contributing/code-style.md)
Expand All @@ -48,12 +52,14 @@ git commit -m "feat: add nitrox calculator"
git commit -m "fix: correct MOD calculation for trimix"
git commit -m "docs: add calculator documentation"
git commit -m "test: add unit tests for gas calculations"
```text
```

### 4. Push

```bash
git push origin feature/your-feature
```text
```

### 5. Open PR

1. Go to your fork on GitHub
Expand All @@ -62,46 +68,64 @@ git push origin feature/your-feature

## PR Template

```markdown
## Description
GitHub pre-fills new PRs from
[`.github/PULL_REQUEST_TEMPLATE.md`](https://github.com/submersion-app/submersion/blob/main/.github/PULL_REQUEST_TEMPLATE.md).
A filled-out description looks like this:

Brief description of what this PR does.
```markdown
## Related Issue

## Type of Change
Closes #123

- [ ] Bug fix
- [ ] New feature
- [ ] Breaking change
- [ ] Documentation
## Summary

## Changes Made
Fixes the MOD calculation for trimix, which used the O2 fraction of air.

- Added X
- Modified Y
- Removed Z

## Testing
## Changes

- [ ] Unit tests added/updated
- [ ] Widget tests added/updated
- [ ] Manual testing performed
- Pass the mix's own O2 fraction into `calculateMod`
- Add unit tests for air, EAN32 and 18/45 trimix

## Checklist
## Test Plan

- [ ] Code follows style guidelines
- [ ] Tests pass locally
- [ ] Documentation updated
- [ ] No new warnings from `flutter analyze`
- [x] `flutter test` passes
- [x] `flutter analyze` passes
- [x] Manual testing on: macOS

## Screenshots

(If applicable)
(Delete this section if not applicable.)
```

## Related Issues
## Linking Issues

Every PR must relate to an issue. The **PR Issue Link** check blocks the merge
until the description links one, and re-runs whenever the description is
edited.

| The PR... | Write | On merge |
| --- | --- | --- |
| fully resolves the issue | `Closes #123` (or `Fixes` / `Resolves`) | the issue closes |
| is one step of a larger issue | `Part of #123` | the issue stays open |
| is related work or a follow-up | `Refs #123` or `Related to #123` | the issue stays open |

Things that trip people up:

- **Only the description counts.** GitHub closes an issue only when a closing
keyword sits directly before the number in the PR description. A number in
the PR title, or a passing mention like "builds on #123", links nothing.
- **One keyword per issue.** `Closes #1, closes #2` closes both;
`Closes #1, #2` closes only #1.
- **Comments and code do not count.** A link inside an HTML comment
(`<!-- -->`) or a code span is ignored, by GitHub and by the check. The
template's examples live in a comment for that reason, so an unedited
template fails.
- **Branch names are checked.** If the branch name contains an issue number
(`issue-123-...`, `github-issue-123-...`, `feature-request-123-...`), the
description must link that issue, with `Refs` if the PR does not resolve it.
- **No issue yet?** Open one first. Only bot-authored PRs (version bumps,
Dependabot) are exempt.

Fixes #123
Related to #456
```text
## PR Best Practices

### Keep PRs Small
Expand All @@ -114,7 +138,7 @@ Related to #456

- Explain what and why
- Include context
- Link related issues
- Link the issue with `Closes #N` or `Refs #N` (see [Linking Issues](#linking-issues))

### Add Screenshots

Expand Down Expand Up @@ -155,7 +179,8 @@ For UI changes:
git add .
git commit -m "fix: address review feedback"
git push origin feature/your-feature
```text
```

## After Merge

### Clean Up
Expand All @@ -169,7 +194,8 @@ git branch -d feature/your-feature

# Update from upstream
git pull upstream main
```text
```

### Celebrate

Your contribution is now part of Submersion!
Expand All @@ -179,6 +205,9 @@ Your contribution is now part of Submersion!
### Bug Fixes

```markdown
## Related Issue
Closes #123

## Description
Fixes incorrect depth unit conversion when switching between metric and imperial.

Expand All @@ -191,10 +220,14 @@ Corrected the conversion factor from 3.28084 to 0.3048 for feet to meters.
## Testing
- Added unit tests for both conversion directions
- Manually verified in settings page
```text
```

### New Features

```markdown
## Related Issue
Closes #456

## Description
Adds a nitrox calculator to the tools section.

Expand All @@ -209,10 +242,14 @@ Adds a nitrox calculator to the tools section.

## Documentation
- Updated tools section in user guide
```text
```

### Refactoring

```markdown
## Related Issue
Refs #789

## Description
Refactors dive repository to use a base repository class.

Expand All @@ -226,7 +263,8 @@ Reduces code duplication across repositories.

## Testing
All existing tests pass without modification.
```text
```

## Common Issues

### Merge Conflicts
Expand Down
Loading