Skip to content

Add Danger check to discourage large PRs #3543

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tonidero merged 5 commits into from
Jun 10, 2026
Merged

Add Danger check to discourage large PRs #3543

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e943a21
Add Danger check to discourage large PRs
tonidero Jun 5, 2026
05d24d9
To revert: Remove a bunch of code to verify failure (it won't even co…
tonidero Jun 5, 2026
168b046
Revert "To revert: Remove a bunch of code to verify failure (it won't…
tonidero Jun 5, 2026
17551f2
Merge branch 'main' into danger-large-pr-check
tonidero Jun 5, 2026
95c2c7f
Merge branch 'main' into danger-large-pr-check
tonidero Jun 9, 2026
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
32 changes: 32 additions & 0 deletions Dangerfile
View file
Open in desktop

@AlvaroBrey AlvaroBrey Jun 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice! Single question here: have you considered adding this to the shared Dangerfile instead?

Especially since we already have check_pr_size_increase (bytes) there

@tonidero tonidero Jun 10, 2026

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.

I thought about it... My main reason to keep it split in each repo is that there are different files/folders we need to analyze on each SDK. Additionally, I think we might want to have a somewhat smaller diff max on some hybrids since most logic lives in the natives, ideally not such big PRs are needed in the hybrids. Of course, we can revisit this later if needed :)

Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,35 @@ revenuecatui_kt.each do |file|
end
end
end

# Fail PRs that change too many lines of production Kotlin/Java code.
# Large PRs are hard to review well, so we cap the churn (insertions + deletions)
# across real .kt/.java files, excluding test sources and generated/build output.
# Authors who legitimately need a large PR can add the bypass label.
PROD_LINES_LIMIT = 300

@tonidero tonidero Jun 5, 2026

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.

Set the limit to 300 right now, which, without including tests, I think it's already pretty big... Lmk if we should aim for lower amounts.

@rickvdl rickvdl Jun 10, 2026

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.

Just checked a few PRs for reference, I think it's a reasonable start and let's tweak if needed :)

SKIP_SIZE_LABEL = "skip-pr-lines-changed-check"

prod_code_files = (git.modified_files + git.added_files).uniq.select do |f|
next false unless f.end_with?(".kt") || f.end_with?(".java")

@tonidero tonidero Jun 5, 2026

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.

Not including gradle files or anything like that... Lmk if you have any thoughts on this!

next false if f.include?("/src/test/") || f.include?("/src/androidTest/") ||
f.include?("/src/testFixtures/")

@tonidero tonidero Jun 5, 2026

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.

Note how in the end, decided to exclude tests... I do think tests are very important, but I'm afraid that if we include tests in the count, we might write less tests...

@rickvdl rickvdl Jun 10, 2026

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 see /purchases/src/testDefaults/.. (same for CEC) wouldn't match the directory check, but it would match the files as long as they end with Test(s).kt/.java so I think we're good. Helper files etc would't be caught, but probably not the biggest deal.

@tonidero tonidero Jun 10, 2026

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.

Good points! Those folders are the minority, since normally most code is part of the common source set. But it's true it's something we will need to keep an eye out in the future if we add more flavors.

next false if f.include?("/build/") || f.include?("/generated/")
next false if f.match?(/Tests?\.(kt|java)$/)
true
end

@cursor cursor Bot Jun 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Deleted production files omitted

Medium Severity

The production line budget only iterates git.modified_files and git.added_files, so wholly removed production .kt/.java files never enter prod_code_files. Their deletions are omitted from total_changed, so large delete-only or delete-heavy refactors can stay under the 300-line cap despite matching the stated insertions-plus-deletions goal.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 17551f2. Configure here.

@tonidero tonidero Jun 5, 2026

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.

Hmm this might be ok? I guess it does depend on the file in the end and how much logic it has... But I think adding this might discourage refactors...

@rickvdl rickvdl Jun 10, 2026

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.

That's fine I'd say, deletions are a lot easier to review as well.


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

if total_changed > PROD_LINES_LIMIT
if github.pr_labels.include?(SKIP_SIZE_LABEL)
message("This PR changes #{total_changed} lines of production Kotlin/Java " \
"(limit #{PROD_LINES_LIMIT}); skipped via `#{SKIP_SIZE_LABEL}` label.")
else
fail("This PR changes #{total_changed} lines of production Kotlin/Java code, " \
"over the #{PROD_LINES_LIMIT}-line limit. Split it into smaller PRs, or add " \
"the `#{SKIP_SIZE_LABEL}` label to bypass.")
end
end