Throttle version bumps based on recently merged PRs.#15286
Throttle version bumps based on recently merged PRs.#15286reitermarkus wants to merge 1 commit intoHomebrew:masterfrom
Conversation
reitermarkus
force-pushed
the
bump-throttle-days
branch
from
e4dcfbc to
3cab08a
Compare
|
Also, not sure this should replace the existing throttle mechanism in Homebrew/core. This is much less transparent for users/contributors: currently, they know to expect a (for example) I'm open to be being persuaded otherwise, though. |
Yeah, that would be better, how do we best check that though?
Too much repository activity is the reason for throttling in the first place, so shouldn't it depend on repository activity? |
Sure, it should. But we also care about UX, and the UX for throttling based on patch versions is better than throttling solely based on repository activity. Patch version increments is also a function of repository activity, so the current throttling method also depends on repository activity. |
How is the UX for |
|
Also, does the livecheck audit need to be skipped for throttled formulae? |
Sure, but that's the same after your change too. What isn't the same is doing |
Not quite what I meant. To clarify, if you run
How so?
I don't get this argument, this is literally how every non-throttled formula works. What is there to work out for users? I doubt users object to a newer patch release if it is already available. Users get whatever version upstream released at the time of a version bump. |
It should probably use patch-version throttling information so that it doesn't fail, then.
Currently,
Not exactly. For most non-throttled formulae, you get new versions shortly after they're released by upstream. For throttled formulae, under the current scheme, you get specific versions shortly after they're released upstream. Under the new scheme, for throttled formulae, you get some updates some of the time, but you don't really know which ones unless you watch Homebrew/core. |
|
To put it a different way: the existing throttling mechanism for formulae is simple, works well, and easily-understandable for users. We even get non-regular contributors making pull requests regarding throttled formulae occasionally: Homebrew/homebrew-core@5a5ce7c Given that, I think the burden is on this change to demonstrate that it is clearly superior to what we're doing now, and I don't think there's really much shown about why that's the case so far. |
|
The PR you linked to wouldn't even be needed if it was based on days rather than patch versions.
I'm not claiming it is clearly superior, I'm claiming it will work just as well for the intended purposes. The question here comes down to: Do we want to maintain two different throttling mechanisms for casks and formulae? My preference would be to use the same for both. |
I don't think you can say that for certain. How do you know that we would've throttled this based on the an appropriate number of days, rather than too few or too many?
Then if it isn't broken: don't fix it. |
|
I think adopting the version number based approach for casks is the better approach here. People already know it, and it's judged on a parameter they can actually predict. Judging when you can make a PR based on what you guess other people will do is near impossible. |
I fail to see how this “multiple of N” approach can work for casks which sometimes don't even have a patch version. How would this even work for date-based versions, dates aren't multiples of N? This approach will work regardless of how weird a cask's versioning scheme is. |
|
For the record, I'm fine with doing this for casks. I'm just not convinced we should do the same for formulae. |
Which casks do you plan on throttling from the outset and what are their version schemes? |
MikeMcQuaid
left a comment
MikeMcQuaid
left a comment
There was a problem hiding this comment.
This needs more discussion before merge.
The |
I mean, it could definitely be done if you're saying e.g. |
| return if formula_suffix.modulo(throttled_rate).zero? | ||
|
|
||
| odie "#{formula} should only be updated every #{throttled_rate} releases on multiples of #{throttled_rate}" | ||
| GitHub.check_recently_merged_pull_requests( |
There was a problem hiding this comment.
I agree that I'd want to see @homebrew/core express a need for this before it's implemented here.
| stable_url_version = Version.parse(stable.url) | ||
| stable_url_minor_version = stable_url_version.minor.to_i | ||
|
|
||
| formula_suffix = stable.version.patch.to_i |
There was a problem hiding this comment.
This should not be removed unless @Homebrew/core agrees the approach in this PR is needed/better.
There was a problem hiding this comment.
Note: I don't agree with changing the throttling scheme for formulae, and I think neither does @SMillerDev (given the 👍 to one of my earlier comments).
There was a problem hiding this comment.
Thanks @carlocab @SMillerDev. Yeh, in that case: let's strip this out please @reitermarkus.
| pr["html_url"].include?("/pull/") && regex.match?(pr["title"]) | ||
| query += " is:pr" | ||
| query += " created:#{created}" if created | ||
| issues_for_formula(query, tap_remote_repo: tap_remote_repo, state: state).select do |issue| |
There was a problem hiding this comment.
Does this use the search API? I assume not but just wanted to check because it's rate-limited so aggressively.
There was a problem hiding this comment.
Yes it does, same as the open/closed PR checks, but it's only called for throttled casks/formulae.
There was a problem hiding this comment.
I don't think it's a great idea to rely on this. As mentioned: the rate limits are already very low and stretched thin.
There was a problem hiding this comment.
We already rely on this for open/closed PRs. Also, this is only called if there is a version bump and it is throttled, i.e. pretty rarely compared to the other checks.
MikeMcQuaid
left a comment
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Ok, I'm fine with this. I'd like to see someone from @Homebrew/core approve the formulae-side changes, though. Have requested a few reviews but anyone else: feel free to chime in.
MikeMcQuaid
requested review from
a team,
MikeMcQuaid,
carlocab,
nandahkrishna and
p-linnane
|
@reitermarkus Needs rebased and then should be good to merge, thanks again! |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
6 participants
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?The existing throttling method (patch version must be a multiple of n) does not work for most casks. This changes throttling to instead check if a PR was merged for a given cask/formula in the last n days instead.