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
pr-pull: allow casks to be pulled #12692
Conversation
Review period will end on 2022-01-11 at 16:59:20 UTC. |
ac23d31
to
4b55244
Compare
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.
Looks good so far!
4b55244
to
1d4c581
Compare
Review period ended. |
92e6b23
to
c57fb56
Compare
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.
What does pr-pull
do for casks? I thought the things it did were pretty formula-specific (download bottles, apply bottle commit, upload bottles). Is it just autosquash?
Just autosquash yeah. But allowing one tool for all parts of homebrew seems practical for automation. |
Agreed. I wonder, though, if it makes more sense to have a Or maybe pull out the bottle stuff from It just feels weird to have such a huge part of this command be formula-specific but also throw in some cask stuff too. I feel like it would be easier to set up workflows and understand what's happening if they're separate. |
This makes sense to me 👍🏻 |
It makes sense to me too, but I don't have the time to split the commands right now. @Rylan12 would you be interested in picking that up? |
Sure, I can spend some time on it. What was your plan for using this with casks, though? Were you thinking we would switch homebrew/cask to the approve-and-automatically-merge workflow that we use in homebrew/core? |
Yeah, that's my dream. Pressing a second button to merge is an annoying deviation from my preferred workflow 😅 |
Sorry for the delay here. Even though I said I could work on it, I forgot that my classes were starting this week so I haven't had any time to work on this. I should next week, though. If you want to merge this, though, I'd be okay with that and can always come back to separating the commands when I have more time. |
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. |
c57fb56
to
4c30ff7
Compare
Okay, I decided to fix this up after all. @Rylan12 and @MikeMcQuaid are you okay merging this despite a new command possibly being cleaner? |
@SMillerDev I'm weakly opposed, personally. I'd be more in favour if I felt it didn't make the code a bit harder to follow. Sorry! ❤️ |
I understand that, I personally think the benefit of a unified automerge for both taps outweigh the added complexity but unless someone else wants to pitch in in either argument or effort I'll leave this one to stalebot to get rid of. |
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.
Changed my mind, fine on this being merged once all comments are addressed.
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.
So sorry for the delay here, and I apologize for not actually having the time to work on this.
I'm also fine with this being merged once the existing comments are addressed. After thinking a little more, I don't think it's super un-intuitive to have it work like this anyway. pr-pull
should pull the PR down and prepare whatever needs to happen, and as is this PR just makes it intelligent enough to do different things based on the type of PR.
4c30ff7
to
4c15f33
Compare
I'm not sure why the caskloader keeps failing now. Anyone have some ideas? |
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. |
fc45cbf
to
ad7bc36
Compare
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.
Some style suggestions as well apart from the get_package
fix.
067ab60
to
54e15cf
Compare
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.
A few style suggestions but: looks good to me to ship whenever! Nice work @SMillerDev!
Co-authored-by: Mike McQuaid <mike@mikemcquaid.com>
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?