Register a SA Forums Account here!
JOINING THE SA FORUMS WILL REMOVE THIS BIG AD, THE ANNOYING UNDERLINED ADS, AND STUPID INTERSTITIAL ADS!!!

You can: log in, read the tech support FAQ, or request your lost password. This dumb message (and those ads) will appear on every screen until you register! Get rid of this crap by registering your own SA Forums Account and joining roughly 150,000 Goons, for the one-time price of $9.95! We charge money because it costs us money per month for bills, and since we don't believe in showing ads to our users, we try to make the money back through forum registrations.
 
  • Post
  • Reply
smackfu
Jun 7, 2004

Personally think people get a bit silly with “please approve” vs “please review.” But that’s developers for you.

Adbot
ADBOT LOVES YOU

prom candy
Dec 16, 2005

Only I may dance
i avoid this whole mess by committing straight to main

Bruegels Fuckbooks
Sep 14, 2004

Now, listen - I know the two of you are very different from each other in a lot of ways, but you have to understand that as far as Grandpa's concerned, you're both pieces of shit! Yeah. I can prove it mathematically.

Aramoro posted:

Start killing people. It's the only way they'll learn.

quote:

Cole: I see deprecated people.

Malcolm: In your dreams?

[Cole shakes his head no]

Malcolm: While you're awake?

[Cole nods]

Malcolm: Deprecated people like, in graves? In coffins?

Cole: Walking around like regular people. They don't see each other. They only see what they want to see. They don't know they're deprecated.

Malcolm Crowe : How often do you see them?

Cole Sear : All the time. They're everywhere.

thotsky
Jun 7, 2005

hot to trot

downout posted:

One of our teams has a bunch of engineers that always ask “please approve this PR!”

Why yes their code reviews are box checking dogshit, why do you ask?

It’s a pretty obvious tell that their entire team culture around code review is terrible.

I have never been on a team with opt-in (ie: no tech lead with sole responsibility for) code reviews where you didn't have to assign, notify and otherwise bug people into actually doing them.

downout
Jul 6, 2009

smackfu posted:

Personally think people get a bit silly with “please approve” vs “please review.” But that’s developers for you.

Fair enough, this team has been driving me nuts because they have been rubberstamping questionable quality commits for a long time. In that context, I think the asking for approval vs asking for code review is emblematic of issues.

smackfu
Jun 7, 2004

Agree there. We’ve definitely had people who considered rubber stamping a PR to count as their work for that hour, without even doing any actual review. Makes a mockery of the process, it does.

xiw
Sep 25, 2011

i wake up at night
night action madness nightmares
maybe i am scum

Cpig Haiku contest 2020 winner

Steve French posted:

There’s an epidemic among my coworkers where they use “deprecated” to mean “removed/deleted” and I can’t handle it.

I've started to see 'demised' used more and more, eurgh.

ThePopeOfFun
Feb 15, 2010

This feature has been taken out back and shot.

Macichne Leainig
Jul 26, 2012

by VG
I just unalived that feature 🫣🫣

darthbob88
Oct 13, 2011

YOSPOS
This module has been sent to a nice farm upstate.

Falcon2001
Oct 10, 2004

Eat your hamburgers, Apollo.
Pillbug

ThePopeOfFun posted:

This feature has been taken out back and shot.

I committed premeditated murder in the first degree on Feature XYZ $ticketlink

CPColin
Sep 9, 2003

Big ol' smile.
I heard "operationalize" once so I nominate "deoperationalize"

Hughlander
May 11, 2005

We have a project with a train metaphor in it and part of the discussion led to talk about left over child processes people were getting annoyed with murder and killing the children, so someone suggested decouple, and it became 'decouple with prejudice'

Bongo Bill
Jan 17, 2012

I'd go with "terminate" if "kill" isn't acceptable.

Volmarias
Dec 31, 2002

EMAIL... THE INTERNET... SEARCH ENGINES...
Yeys hallo I vood lyke to liqvidate seferal chillldren pleas und zank yu.

Nybble
Jun 28, 2008

praise chuck, raise heck

Bongo Bill posted:

I'd go with "terminate" if "kill" isn't acceptable.

just have to put it in monotype:

code:
kill -9 project_name

Fellatio del Toro
Mar 21, 2009

fatal error: attempted to decouple child before execution

Bruegels Fuckbooks
Sep 14, 2004

Now, listen - I know the two of you are very different from each other in a lot of ways, but you have to understand that as far as Grandpa's concerned, you're both pieces of shit! Yeah. I can prove it mathematically.

Bongo Bill posted:

I'd go with "terminate" if "kill" isn't acceptable.

that term is actually more strongly associated with "kill" than "kill" for me. if only there weren't a popular movie series featuring a robot that goes around terminating people by that name.

Falcon2001
Oct 10, 2004

Eat your hamburgers, Apollo.
Pillbug

Bruegels Fuckbooks posted:

that term is actually more strongly associated with "kill" than "kill" for me. if only there weren't a popular movie series featuring a robot that goes around terminating people by that name.

Now I'm imagining an alternative world where the series is just called KILLER and it's pretty amusing.

Hughlander
May 11, 2005

Bruegels Fuckbooks posted:

that term is actually more strongly associated with "kill" than "kill" for me. if only there weren't a popular movie series featuring a robot that goes around terminating people by that name.

That's why I suggest EXTERMINATE

Volmarias
Dec 31, 2002

EMAIL... THE INTERNET... SEARCH ENGINES...

Hughlander posted:

That's why I suggest EXTERMINATE



Sorry, that one has been done

MadFriarAvelyn
Sep 25, 2007

Work has all SWE staff going through a three hour long training program that commits the ultimate training program sin of unskippable video tutorials.

The entirety of it amounts to "review the OWASP top 10."

We now have to do this same training every quarter.

:negative:

Volmarias
Dec 31, 2002

EMAIL... THE INTERNET... SEARCH ENGINES...

MadFriarAvelyn posted:

Work has all SWE staff going through a three hour long training program that commits the ultimate training program sin of unskippable video tutorials.

The entirety of it amounts to "review the OWASP top 10."

We now have to do this same training every quarter.

:negative:

I hope you can place it in another window and mute it.

lifg
Dec 4, 2000
<this tag left blank>
Muldoon
Does it have little quizzes after each section? I hate that. But I did learn that with some of the videos I could just see a full transcript and quickly read that.

Hughlander
May 11, 2005

lifg posted:

Does it have little quizzes after each section? I hate that. But I did learn that with some of the videos I could just see a full transcript and quickly read that.

I have a note in obsidian with the answers to each of those quizzes from previous times since they don't randomize questions or answers.

smackfu
Jun 7, 2004

The worst is when you have to get all the questions correct in the pre-quiz to skip the training and one of them is like “how many days do you have to respond to a reported security incident?” And the options are completely arbitrary.

Jabor
Jul 16, 2010

#1 Loser at SpaceChem
Ask if they have a transcript version available for the hearing-impaired, and if you can read that instead of watching the video.

If the answer is yes, you've saved yourself some time - if the answer is no, things might get even more entertaining!

smackfu
Jun 7, 2004

In Connecticut, you have to do two hours of anti discrimination / harassment training, which works fine if it’s a class. We have online training so if you finish before two hours, it just shows a screen with a countdown timer and a disabled Continue button.

Great job.

Xarn
Jun 26, 2015

thotsky posted:

I have never been on a team with opt-in (ie: no tech lead with sole responsibility for) code reviews where you didn't have to assign, notify and otherwise bug people into actually doing them.

Tech lead with sole responsibility for reviews sounds ghastly. And assigning/notifying people is normal, do you expect everyone to just look at all open PRs at all times to write notes? How do they know that the PR is ready? How do they know the PR should interest them?

Itaipava
Jun 24, 2007
Agree that assigning people to PRs is good, one of my bookmarks is the "assigned to me" list on Github. Bugging people to actually do the reviews is so annoying though, I feel like an overbearing parent reminding people of their own work

Volmarias
Dec 31, 2002

EMAIL... THE INTERNET... SEARCH ENGINES...
There was a tool at my last job that would auto assign review requests to people in the team (round robin or something). You could always kick it to someone else if you needed to, but it was a great way to prevent people's changes from languishing, or from the same people reviewing over and over.

Hughlander
May 11, 2005

One I kind of liked was based on the area of the code people were assigned as required automatically, when there were no required people that didn't sign off you submitted. Culturally if someone didn't engage in a certain amount of time they were moved from required to optional.

Clanpot Shake
Aug 10, 2006
shake shake!

Am I the odd one out for enjoying PRs? I find they're one of the best places to spend time if your goal is increasing code quality.

downout posted:

One of our teams has a bunch of engineers that always ask “please approve this PR!”

Why yes their code reviews are box checking dogshit, why do you ask?

It’s a pretty obvious tell that their entire team culture around code review is terrible.

Culture is a hard thing to change. They misunderstand the point of a PR. The question the reviewer is answering is not "does this do the thing?" - the author asserted that it does the thing when they opened the PR. The question is "can this be made better?" which can almost always be answered with "yes." From there the conversation can be about the relative effort and value of any proposed changes, and by that collaboration you will nearly always walk away with an improved solution. And who knows, somebody might learn something

thotsky
Jun 7, 2005

hot to trot

Xarn posted:

Tech lead with sole responsibility for reviews sounds ghastly. And assigning/notifying people is normal, do you expect everyone to just look at all open PRs at all times to write notes? How do they know that the PR is ready? How do they know the PR should interest them?

I vastly prefer having a tech lead with the sole, or at least primary responsibility for performing code reviews. Otherwise it just takes too long to get them done. Developers who have tickets of their own are not going to drop whatever they're doing to review mine, and ideally I want to have several PRs go through in a single workday.

I have worked at places where code reviews were meant to be handled communally by people checking the open PR page whenever they were not working on something, which resulted in them never getting done. I agree that bugging people about PRs is the norm; downout is the dude who seems to think that's weird.

Also, you don't post PRs before they're ready. If you absolutely have to (horrible, means it's a branch that's definitely living too long or that what you really want to do is a bit of pair programming) you mark them with WIP.

thotsky fucked around with this message at 22:23 on Oct 7, 2023

Volmarias
Dec 31, 2002

EMAIL... THE INTERNET... SEARCH ENGINES...

Clanpot Shake posted:

Am I the odd one out for enjoying PRs? I find they're one of the best places to spend time if your goal is increasing code quality.

Culture is a hard thing to change. They misunderstand the point of a PR. The question the reviewer is answering is not "does this do the thing?" - the author asserted that it does the thing when they opened the PR. The question is "can this be made better?" which can almost always be answered with "yes." From there the conversation can be about the relative effort and value of any proposed changes, and by that collaboration you will nearly always walk away with an improved solution. And who knows, somebody might learn something

More succinctly, the purpose of the code review is to be the author, six months later, asking "what stupid rear end in a top hat wrote this," except that you can gently point things out before they get committed to the indelible ledger of shame that is your source control repository.

Plorkyeran
Mar 22, 2007

To Escape The Shackles Of The Old Forums, We Must Reject The Tribal Negativity He Endorsed

Clanpot Shake posted:

Am I the odd one out for enjoying PRs? I find they're one of the best places to spend time if your goal is increasing code quality.

Depends on who the PR is from. Reviewing PRs from people who actually want to have their code reviewed and are eager for feedback can be both productive and satisfying. Reviewing PRs from people who get offended if you suggest that their code is not absolutely perfect is incredibly frustrating.

Many developers just plain don't like to read code and reviewing PRs is nothing but reading code.

Xarn posted:

Tech lead with sole responsibility for reviews sounds ghastly. And assigning/notifying people is normal, do you expect everyone to just look at all open PRs at all times to write notes? How do they know that the PR is ready? How do they know the PR should interest them?

You check for PRs at the start of the day and then later when you have a gap. You know that a PR is ready because it exists and isn't marked as a draft. In a small team every PR is relevant to you because you shouldn't be siloed off with like four people. Obviously in a larger team you need more structure, but it's really not very complicated.

Volmarias
Dec 31, 2002

EMAIL... THE INTERNET... SEARCH ENGINES...

quote:

Many developers just plain don't like to read code and reviewing PRs is nothing but reading code.

It's actually very nice, because you can say "please add tests" and you do not have to write the tests yourself

downout
Jul 6, 2009

thotsky posted:

I vastly prefer having a tech lead with the sole, or at least primary responsibility for performing code reviews. Otherwise it just takes too long to get them done. Developers who have tickets of their own are not going to drop whatever they're doing to review mine, and ideally I want to have several PRs go through in a single workday.

I have worked at places where code reviews were meant to be handled communally by people checking the open PR page whenever they were not working on something, which resulted in them never getting done. I agree that bugging people about PRs is the norm; downout is the dude who seems to think that's weird.

Also, you don't post PRs before they're ready. If you absolutely have to (horrible, means it's a branch that's definitely living too long or that what you really want to do is a bit of pair programming) you mark them with WIP.

I don't think it's weird, I think it's a problem when it's treated as a check box and no review is happening. "approval" vs "review"

edit: the bugging people is fine, gotta let people know somehow that there is a task for them to do.

downout fucked around with this message at 00:18 on Oct 8, 2023

StumblyWumbly
Sep 12, 2007

Batmanticore!
In general, if you ask a room of people to something that is more complex or time consuming than to water a plant, you will not get a response. It does not matter how many people are in the room.

Adbot
ADBOT LOVES YOU

Jabor
Jul 16, 2010

#1 Loser at SpaceChem
Pretty much. You need to have a system that asks a specific person to do the review, and a team culture that expects them to do their assigned reviews in a somewhat timely fashion.

  • 1
  • 2
  • 3
  • 4
  • 5
  • Post
  • Reply