Skip to content

Add Danger check to discourage large PRs#7041

Merged
tonidero merged 3 commits into
mainfrom
danger-discourage-large-prs
Jun 18, 2026
Merged

Add Danger check to discourage large PRs#7041
tonidero merged 3 commits into
mainfrom
danger-discourage-large-prs

Conversation

@tonidero

@tonidero tonidero commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Why

Large PRs are slow and error-prone to review, and problems are more likely to slip through. This adds a Danger check that nudges us toward smaller, more focused PRs.

What

Fails the Danger check when a PR changes more than 300 lines of production Swift code (insertions + deletions). Test sources, example apps, and generated output are excluded so only "real" code counts.

When a large PR is genuinely necessary, add the skip-pr-lines-changed-check label to bypass the failure.

Mirrors the equivalent Android check: RevenueCat/purchases-android#3543


Note

Low Risk
CI/documentation only; no SDK runtime or public API behavior changes.

Overview
Adds a Danger gate that fails when a PR’s production Swift churn (insertions + deletions) exceeds 300 lines, counting only Sources/, RevenueCatUI/, and RulesEngineInternal/ while excluding tests, example apps, and Sources/Generated/.

Authors can bypass with the skip-pr-lines-changed-check label (warns instead of failing). AGENTS.md is updated to match the ~300-line guideline and to call out test/example exclusions.

Reviewed by Cursor Bugbot for commit 08d4474. Bugbot is set up for automated code reviews on this repo. Configure here.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Comment thread Dangerfile

COMMENT_MARKER = "<!-- purchases-ios-danger -->"

PROD_LINES_LIMIT = 300

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do lmk what you think of this limit... I feel that in iOS, changes tend to be more verbose, so we might want to increase this limit... but maybe we can see how it works with this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Let's start with this and iterate if it gets too noisy. I also think that excluding certain files (as you already did) should make things better

@tonidero
tonidero marked this pull request as ready for review June 18, 2026 13:58
@tonidero
tonidero requested a review from a team as a code owner June 18, 2026 13:58

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit d514d26. Configure here.

Comment thread Dangerfile
total_changed = prod_files.sum do |f|
info = git.info_for_file(f)
info ? info[:insertions] + info[:deletions] : 0
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleted Swift omitted from limit

Medium Severity

pr_size_messages only sums churn from git.modified_files and git.added_files, so production Swift removed via git.deleted_files is ignored. A PR that deletes hundreds of lines in Sources/ or RevenueCatUI/ can pass the check while still being a large review, contrary to the stated insertions-plus-deletions rule.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit d514d26. Configure here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was discussed in the Android PR and was accepted to allow for some refactors. If the entire file is removed, it's probably ok

Comment thread Dangerfile
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@ajpallares ajpallares left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you (thank you really) for adding this!!

We should probably also update the number in

- **Prefer smaller, self-contained stacked PRs** when possible. "Self-contained" means the PR compiles, passes tests, and is reviewable on its own. As a rule of thumb, aim to keep the diff under ~500 lines of hand-written source — excluding generated, serialized, or non-reviewed files (e.g. JSON, snapshots, lockfiles, `api/*.swiftinterface`). For larger tasks, plan the PR breakdown up front and stack interdependent changes instead of bundling everything into one large PR.

which I originally set to ~500 in #6915

Comment thread Dangerfile

COMMENT_MARKER = "<!-- purchases-ios-danger -->"

PROD_LINES_LIMIT = 300

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Let's start with this and iterate if it gets too noisy. I also think that excluding certain files (as you already did) should make things better

@tonidero
tonidero enabled auto-merge (squash) June 18, 2026 14:39
@tonidero
tonidero merged commit 2c663d0 into main Jun 18, 2026
18 of 20 checks passed
@tonidero
tonidero deleted the danger-discourage-large-prs branch June 18, 2026 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants