-
Notifications
You must be signed in to change notification settings - Fork 58
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.
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
Add riskiness label when creating rollups #138
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,3 +7,4 @@ | |
/main.db | ||
/cache | ||
*.pyc | ||
.vscode/ |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -101,6 +101,17 @@ | |
background: #F0DE57; | ||
border-color: #8C7E14; | ||
} | ||
.riskiness_low { | ||
color: #01CE01; | ||
} | ||
.riskiness_medium { | ||
color: #822a00; | ||
font-weight: 600; | ||
} | ||
.riskiness_high { | ||
color: #de0000; | ||
font-weight: 900; | ||
} | ||
|
||
.sorting_asc:after { content: " ▲"; } | ||
.sorting_desc:after { content: " ▼"; } | ||
|
@@ -136,6 +147,7 @@ <h1>Homu queue - {% if repo_url %}<a href="{{repo_url}}" target="_blank">{{repo_ | |
|
||
<div id="actual-rollup" class="hide"> | ||
<p>This will create a new pull request consisting of <span id="checkbox-count">0</span> PRs.</p> | ||
<p>Based on the number of PRs and their rollup statuses, this rollup is considered <span id="riskiness">not risky</span>.</p> | ||
<p>A rollup is useful for shortening the queue, but jumping the queue is unfair to older PRs who have waited too long.</p> | ||
<p>When creating a real rollup, see <a href="https://forge.rust-lang.org/release/rollups.html">this instruction</a> for reference.</p> | ||
<p> | ||
|
@@ -222,9 +234,38 @@ <h1>Homu queue - {% if repo_url %}<a href="{{repo_url}}" target="_blank">{{repo_ | |
]; | ||
|
||
document.getElementById('expand-rollup').onclick = function() { | ||
var checkboxCount = document.querySelectorAll('#queue tbody input[type=checkbox]:checked').length; | ||
var els = document.querySelectorAll('#queue tbody input[type=checkbox]:checked'); | ||
var checkboxCount = els.length; | ||
document.getElementById('checkbox-count').innerHTML = checkboxCount; | ||
document.getElementById('actual-rollup').className = ''; | ||
|
||
var value = 0; | ||
for (var i = 0; i < els.length; i++) { | ||
var rollupClassName = els[i].parentNode.parentNode.children[10].className; | ||
if (rollupClassName === "rollup_always") { | ||
} else if (rollupClassName === "rollup_iffy") { | ||
value += 2; | ||
} else { // nothing or "rollup_maybe" | ||
value += 1; | ||
} | ||
} | ||
value = (value / 8) + (Math.max((checkboxCount - 8), 0) / 8); | ||
|
||
var riskiness = 'not risky'; | ||
var riskinessClass = 'riskiness_low'; | ||
if (value > 0 && value <= 0.5) { | ||
riskiness = 'acceptably risky'; | ||
} else if (value <= 1.0) { | ||
riskiness = 'somewhat risky'; | ||
} else if (value <= 2.0) { | ||
riskinessClass = 'riskiness_medium'; | ||
riskiness = 'risky'; | ||
} else if (value > 2.0) { | ||
riskinessClass = 'riskiness_high'; | ||
riskiness = '⚠️very risky⚠️'; | ||
} | ||
Comment on lines
+256
to
+266
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These limits are super low, and I'm worried that since they'll trigger on almost every PR people will just ignore them. Here's some rollups that landed in the last few days:
None of these PRs are anything less than "very risky"; I doubt you'll find any that are in the past few weeks (there's a list at https://github.com/rust-lang/rust/pulls?q=is%3Apr+label%3Arollup; finding the riskiness is manual). Maybe as an alternative you could raise the limit to something like 4, and mark There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Honestly even the rollups I linked don't seem super risky to me, I would mark the 8 PR rollup as "riskiness_medium" and the rest as "riskiness_low". "riskiness_high" would be rollups with over 12 PRs (and I have some monsters with over 20 I could link too). |
||
document.getElementById('riskiness').innerHTML = riskiness; | ||
document.getElementById('riskiness').className = riskinessClass; | ||
}; | ||
|
||
document.getElementById('cancel-rollup').onclick = function() { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practice no one will ever make a rollup with exactly 0 risk, I think you should just use "not risky" or "low risk" for both these cases. "acceptably risky" seems to imply the others are unnacceptably risky which I don't think is the right message.