|
PRs are really good for your giant open sores project with six vendors, six hundred contributors, and an Apache org. in that context it's always more important to have a change vetted and approved by all stakeholders than it is to avoid wasting the time of developers PRs are considerably less good for your six man team all working on the same project every week
|
#
?
May 22, 2020 00:22
|
|
|
|
| # ? Apr 26, 2024 22:52 |
|
|
you know that prs don't have to be across forks, right. they can be to merge branch a into branch b (in the same repo). and most people use them only half for the making sure that people can't push willy-nilly part - the other part is that most providers that support a pull request like concept also provide a nice online way to make comments on code, review them, have conversations, etc that is much better than wheeling your chair over for 15 minutes and letting your eyes glaze over while Jerry talks about poo poo you don't care about and you miss some important bug
|
|
#
?
May 22, 2020 00:49
|
|
|
lol
|
|
#
?
May 22, 2020 00:49
|
|
|
Notorious b.s.d. posted:PRs are really good for your giant open sores project with six vendors, six hundred contributors, and an Apache org. in that context it's always more important to have a change vetted and approved by all stakeholders than it is to avoid wasting the time of developers not when you have people actually read them and give meaningful feedback and suggestions. doing that has made me a slightly less shameful and terrible programmer from being at both ends of that
|
|
#
?
May 22, 2020 00:52
|
|
|
Notorious b.s.d. posted:i fuckin hate pull request workflow really showing your whole rear end about how little you know right here lol
|
|
#
?
May 22, 2020 01:24
|
|
|
Notorious b.s.d. posted:i fuckin hate pull request workflow one thing that’s actually very cool about k8s at my job is how easy it makes it for us to run the integration test suite against a full-up backend for every PR. also we land dozens of PRs each day, and it’s pretty rare for a substantial PR to get approved without changes. what is going on at these workplaces that no one is willing to merge to master
|
|
#
?
May 22, 2020 01:26
|
|
|
I now have access to years and years of video of old people waving their hands about and smoking cigars, Something using GraphQL for queries with wonderfully long URL encoded parameters, JavaScript code:
|
|
#
?
May 22, 2020 01:39
|
|
|
lmao
|
|
#
?
May 22, 2020 02:17
|
|
|
ctps: There's a specific workflow that involves passing things in and out of json/mongo/strings/python dictionaries. This includes data fields that could be empty, as well as image data which has been base64 encoded. A "strange and rare failure mode" was debugged and solved this week and it was discovered that replacing the characters "null" for "None" in the "simplest" way possible does indeed end up finding those characters in some small fraction of the coded images and wreck them up.
|
|
#
?
May 22, 2020 03:00
|
|
|
Notorious b.s.d. posted:i fuckin hate pull request workflow Bloody posted:really showing your whole rear end about how little you know right here lol I’d describe it as “showing your whole rear end re: working anywhere that people are competent enough to run ‘git pull —rebase’”
|
|
#
?
May 22, 2020 03:51
|
|
|
IMO he's right that pushing straight to master is better for small teams/projects (I would say 3 or 4 people; 6 is probably pushing it). PRs mostly just waste everyone's time and increase merge conflict risk under those circumstances.
|
|
#
?
May 22, 2020 05:01
|
|
|
I’m not even a developer and i know enough to know that nbsd post was garbage lmao
|
|
#
?
May 22, 2020 05:02
|
|
|
of course "permission to push to master" in git actually means "permission to irreparably destroy your entire repo" but that's a git problem not a workflow one
|
|
#
?
May 22, 2020 05:02
|
|
|
cool av posted:of course "permission to push to master" in git actually means "permission to irreparably destroy your entire repo" but that's a git problem not a workflow one ??? GitHub protected branches feature?
|
|
#
?
May 22, 2020 05:11
|
|
|
Phobeste posted:you know that prs don't have to be across forks, right. they can be to merge branch a into branch b (in the same repo). and most people use them only half for the making sure that people can't push willy-nilly part - the other part is that most providers that support a pull request like concept also provide a nice online way to make comments on code, review them, have conversations, etc that is much better than wheeling your chair over for 15 minutes and letting your eyes glaze over while Jerry talks about poo poo you don't care about and you miss some important bug They are also a place to run your ci pipeline before you poo poo up master with your code. I actually have push-to-master rights at my job, and I stopped using them, because in 9/10 cases the "trivial, obviously right" change wasn't.
|
|
#
?
May 22, 2020 07:08
|
|
|
Notorious b.s.d. posted:i fuckin hate pull request workflow Keep the PRs small, and create one often. The previous year I averaged about 1 each day. If your PRs are huge they’ll be a huge pain to review, we tried to make one for each task basically and tasks that are bigger than a day are should be unusual. And if it doesn’t get reviewed the same day it’s the first thing that happens tomorrow. The only time a PR stays open for longer than a day is for the first few when onboarding a new developer that might have a lot of things to learn. Having a good pipeline is essential because you want to run your CI against the PR before it’s merged! If you get bikeshedding comments either just do it if it’s no big deal and fixing it is fast so you can get done. Or resolve the comment with Won’t fix and the reviewer can decide if they want to withhold approval over meaningless bullshit, they probably won’t. If they do and are senior then it’s probably them wanting to contribute “something”, humor them or fight it your call. I think the key to getting this to work is a combination of technical things, like enforced policies for each PR regarding rests &c, and social. Everyone knows how annoying it is to feel “benched” while waiting for your current PR to close so you can devote yourself to your next task, so when a co-worker opens a PR you review it pronto.
|
|
#
?
May 22, 2020 07:32
|
|
|
cool av posted:IMO he's right that pushing straight to master is better for small teams/projects (I would say 3 or 4 people; 6 is probably pushing it). PRs are good for small teams as well as they build in Code Reviews to the work flow. And code reviews are important because programmers are garbage.
|
|
#
?
May 22, 2020 08:21
|
|
|
a fresh set of eyes on a piece of work is rarely a bad thing* *notable exception being a rockstar reviewing your ticket and turning <10 changed lines into a major rearchitecting because *~reasons~*
|
|
#
?
May 22, 2020 08:57
|
|
|
jesus WEP posted:a fresh set of eyes on a piece of work is rarely a bad thing* "sorry, but unfortunately we don't have enough time left for a comprehensive refactoring. i'll merge this pr and put in a tech debt ticket"
|
|
#
?
May 22, 2020 09:19
|
|
|
redleader posted:"sorry, but unfortunately we don't have enough time left for a comprehensive refactoring. i'll merge this pr and put in a tech debt ticket" not only that but when your comprehensive refactoring introduces a subtle bug that no-one notices for 2 months, it’s a fuckload easier to find it when the change isn’t hidden away in a commit labeled “YOSPOS-219 change button to green”
|
|
#
?
May 22, 2020 09:46
|
|
|
redleader posted:"sorry, but unfortunately we don't have enough time left for a comprehensive refactoring. i'll merge this pr and put in a tech debt ticket" mm yeah Tech Debt, get round to that anytime now 
|
|
#
?
May 22, 2020 10:21
|
|
|
Aramoro posted:mm yeah Tech Debt, get round to that anytime now i'm the A rating
|
|
#
?
May 22, 2020 10:24
|
|
|
Chalks posted:i'm the A rating Our ratio is still less than 5% so it's A OK!
|
|
#
?
May 22, 2020 10:34
|
|
|
jesus WEP posted:a fresh set of eyes on a piece of work is rarely a bad thing* Yeah enforced PRs are great except for this, I often ended up going to specific team members for reviews b/c I knew some people were really poo poo that might be more of a management issue and I haven't had to deal with it as much as I became more senior e: to be clear the people I was avoiding were ones with around 2-3 years of experience, I find that more experienced devs are by and large pretty laid back about the whole thing Private Speech fucked around with this message at 10:47 on May 22, 2020 |
|
#
?
May 22, 2020 10:42
|
|
|
zokie posted:Having a good pipeline is essential because you want to run your CI against the PR before it’s merged! forcing computers to look at and deal with what they have shamefully enabled is the only just thing that's happened in the last 10 years
|
|
#
?
May 22, 2020 11:02
|
|
|
unless it's a toy project that only you work on or it's like a wrapper to package libraries or something don't commit direct to master imho, branch and merge and tag the merge points on master kind of in two minds on squashing though, not doing it provides the history on master of "how did this get hosed up" at a detailed level, but this also completely bloats the history out. jesus WEP posted:yeah lol this is one of the things I blew up about last week, 30 commits just labelled "implementation" with no further detail on what they were doing or why. edit: so I guess forcing a PR and squashing those would have been fine because its not like we could figure out what the gently caress was going on from the individual commits anyway
|
|
#
?
May 22, 2020 12:11
|
|
|
last two jobs I had a hell of a time getting people to review my PRs to the point that at last job after a couple days is declare "last call" and ok it myself 24 hours later didn't help that the first few months I was there the guy I was supposed to be working with didn't actually know what a pr was
|
|
#
?
May 22, 2020 12:11
|
|
|
We use Phabricator as a little bit of custom scripting to assign code reviews to people so there's just a big open list of things it's hard to ignore.
|
|
#
?
May 22, 2020 12:18
|
|
|
Aramoro posted:mm yeah Tech Debt, get round to that anytime now 
|
|
#
?
May 22, 2020 12:32
|
|
|
Xarn posted:They are also a place to run your ci pipeline before you poo poo up master with your code. yup. I have those rights too because we’re a small enough shop that we do our own admin and I’m honestly never tempted. it’s so much better to have other people see it, to run tests you might have forgotten, run tests on other OSs, etc etc. my last job was two devs for the fw side and had no unit testing or linting or static analysis and didn’t really do prs, and it was painful - bugs could only be caught through (admittedly quite robust) HIL integration testing or through test-it-on-your-desk and it definitely slowed us down constantly. I was making progress on getting us to code review culture when I remembered I hated working there and quit lol
|
|
#
?
May 22, 2020 13:01
|
|
|
ive never seen ‘the pr workflow’ hosed up badly enough to require weekly meetings
|
|
#
?
May 22, 2020 13:39
|
|
|
Aramoro posted:We use Phabricator as a little bit of custom scripting to assign code reviews to people so there's just a big open list of things it's hard to ignore. we use phab too and I find it extremely easy to ignore
|
|
#
?
May 22, 2020 14:18
|
|
|
Powerful Two-Hander posted:
If your coworkers cannot produce hygienic commits to save their lives (half of mine cannot), it is better to squash-merge than to not. If your coworkers understand atomic commits and rebasing, don't squash commits. EZ
|
|
#
?
May 22, 2020 15:36
|
|
|
squashing is easier and faster. if your massive patch train is incomprehensible as a single commit, try breaking it up into multiple PRs
|
|
#
?
May 22, 2020 15:48
|
|
|
Notorious b.s.d. posted:i fuckin hate pull request workflow the PR workflow is the only good thing in the poo poo accretion disk that orbits the supermassive effort black hole that is git everyones job is different and there's no real objectively wrong way to do stuff, but you're getting really close to discovering one imo.
|
|
#
?
May 22, 2020 19:24
|
|
|
jesus WEP posted:a fresh set of eyes on a piece of work is rarely a bad thing* read this as rear-architecting because i'm tired butt it works
|
|
#
?
May 22, 2020 19:41
|
|
|
Brain Candy posted:butt it works actually,
|
|
#
?
May 22, 2020 19:44
|
|
|
rotor posted:supermassive effort black hole mods
|
|
#
?
May 22, 2020 19:46
|
|
|
It would have been nice if Mercurial won instead of Git but it didn't so here we are. I'm happy as long as I never have to touch Subversion or Perforce ever again. Thank god at least ClearCase seems to be utterly extinct.
|
|
#
?
May 22, 2020 19:51
|
|
|
|
| # ? Apr 26, 2024 22:52 |
|
|
Remember bitkeeper? Me either
|
|
#
?
May 22, 2020 19:53
|
|
