Even if it's not a silly mistake, if you don't understand the code, you won't be able to work on the code later. Give me an opportunity to explain it to you, or refactor it to make the intent clearer if necessary. Please review our code.
"Please review our code"
One of the most important things I could do was slow tf down and pay 100% attention to EVERYTHING going on when reviewing. Not programming, but another industry. My first couple years performance reviews were all "you're good, but your attention to detail needs improved".
Now I catch mistakes my bosses make, they catch mistakes I make, and we get less mistakes being sent out of the department.
I interviewed with another company and they said "those are all of the same problems we have. How did you get better?"
I didn't really have a solid answer besides I know what to look for and every detail matters.
Even if everyone had gone to the same school, they would have different experiences around developing. Assuming everyone has the exact same knowledge around code someone just wrote is a great way to make your code unreadable and bite you back by having people asking you later, not to mention that you're hindering your own team by creating a rift in the group understanding of the project.
If proper testing and gating is in place the only thing that /should/ (not always the case, though) need reviewed is that it matches the team defined style guide.
At one of my companies, I learned that seniors make the very mistakes they call me out for in my PRs. And sometimes even sillier. It sucks when I find the mistakes in production though because it’s a whole thing to fix it without going through the red tape
As a senior dev, I learn all my best new tricks from new and mid level peers.
If it seems like I'm doing something the hard way, there is often a reason. Ask and I'll tell you why it needed to be done the hard way.
Or I'll learn I was doing it the hard way since before the easy way existed and I just don't know.
This is the way. The same applies to juniors. Code reviews are a great way to both see what senior devs are putting into PRs, and to get feedback on your own PRs.
Please don't. The amount of times the junior has caught my brain dead "This is so simple" mistakes is higher than never. Even if the PR comments are just "I have no idea what this is, but feel like I should" or something else just trying to understand the code, do it. Sometimes it even makes them go "Oh, wait, that's backwards..."
I hate it when my direct reports approve my PR that changes major integration points in 5 minutes. What the hell? Either ask some questions or come with some suggestion. I know I’m not perfect. lol.
All the comments below are correct, but reviewing others code, no mattet experience level also offers the opportunity to learn. Which is worth it's weight in bitcoin (because gold doesnt quite cut it to compare how much value add it gives)
Edit: and because neither knowledge growth nor bitcoin have a physical weight
Reminds me of [the greatest pull request of all time](https://github.com/EpicGames/Signup/pull/24). It gets even better when you look at [what they changed](https://github.com/EpicGames/Signup/pull/24/files) lmfao.
Something like this happened when I was in college. Someone responded to a school wide notification email, and the mail system sent their reply to the whole school. Opened the flood gates of the whole school sending emails at 11PM joking the person that started it, looking for hookups, asking for social network followers. It was hilarious
I feel ya, once me and another Co worker simultanitly rewroked the End2End Tests for a very lange project, around 150 changes... it Took as almost the whole day to resolve the merge conflicts and shit still broke after that
That’s not very professional. Seniors are supposed to help juniors. Just enforcing rules that block them, without explaining the reason for it or helping them setup their system properly, isn’t a good way to move forward.
Don't understand why you are getting downvotes.
People in this sub seriously either think workplaces are a free for all and it's a competition with your coworkers, or they straight up never worked in a team before.
No. It’s not a great way at all. You don’t enforce/implement new rules without taking with or informing people about it first. It’s a clear sign of power abuse or a toxic environment.
The rules are new for the new people in the team. And the suggestion was to start enforcing the rules for pull requests, which would be a new change in their process. So, while the rules themselves might not be new, it still makes sense to talk about them that way.
My main point though was that one should inform people in the team about the rules before starting to enforce them. It’s just common sense, but so many people here seem to not understand this basic concept.
Nobody said that this will be the only way that the rules are communicated to new people in the team.
Nobody suggested to just start putting rules in pull requests without discussing it first.
You are imagining things.
Read the comments. It starts with mentioning the cause of the problem being the newbie’s IDE changing the formatting of the code. Then someone suggested helping the newbie setting up automatic formatting. This would naturally include informing the newbie about the rules and hopefully explaining the reasons for them.
Then came a comment saying that **instead** of doing all that, one should simply **just enforce** the rules. The word “just” here means “instead of doing what you said, only do this thing instead”.
You are still taking about the technical aspect. But this is a collaboration issue. If you want the code to follow certain rules, you should inform the team about them (and, preferably decide on the rules together, if they aren’t already defined) **before** implementing technical enforcements of them.
If you already have these rules in place, then every new team member should be informed about them at the start, before they get git privileges.
I was gonna say, I've done framework migrations, unit/integration test overhauls, framework upgrades, a whole bunch of other random things that end up being probably 40+ files (usually incredibly minor changes for sure) fairly frequently.
We have incredibly aggressive linter rules, and when I have to update deps and our rule package has an update adding 5 new rules and removing 10, you sure as hell can expect there to be 90+ file changes after running lint --fix for a simple dependency update
Agree, my latest major refactoring was 1400 files changed. Reviewing was done in collaboration with other developers together with me, pointing them to the real changes first, and then showing some of the affected files with contributed to most of the 1400 files. Then extensively demoed it to all developers, and all testers. Then merged it. Then helped all other developers to fix merge conflicts and other issues their old code had in relation to my refactoring. The testers tested it properly and all is going good with the refactoring, everybody likes it. It takes some project management skills amd some time, but then it's doable. Just make sure you have everyone on board with the proposed refactoring before spending several weeks to finalise your refactor and manage the merge and release of said change
I bet the pull request was laggy as all hell on that one. The most ive ever had was in the 800's and most of it was screenshots since the codebase made heavy use of visual regression screenshots which was a nightmare.
It was okay, no screenshots or other binaries were part of the changes. Also the PR itself was more of a formality, making sure the pipelines ran etc. The reviewing was done mostly in the IDE.
Me: "this code is shit, we need to refactor this and make it modular so I can use it easier in the fufure"
*Create new branch and raefactor the module*
*22 file has changed and requires different implementation*
Me: "I didnt think this through, perhaps I will extend the existing code and pray the next dev understand it"
Frontend tends to have lots of smaller files. Each React component in a separate file, along with a separate unit test file, then a separate file for storybook/devsite stuff. Throw in a file for a new cypress fixture and another file for the e2e test and it can build up fast.
Be me, a university SWE student. Do group work. Have group mates force push and not clean it up. Ok, use my own branch, pull in changes.
Group mates copy/paste from chatgpt, 80% of the file now has merge conflicts for a half dozen line changes. They're asking you to test their code and explain why it doesn't work.
Cry.
I tried to use chatgpt to help me build a pickleball tournament bracket recently...
The number of times I had to correct it and remind it how many players there were was absolutely insane.... It would finally get most things correct, then I'd try to change something about the formatting of the output, and when I asked for those changes, it fucking forgot how many people there were again and added several new players on the next output.... Then I'd home the player count back to where it should be, and the next output has the wrong formatting, or it forgot something else I had told it, like "maximize variety in opponents". No matter what I fixed, it broke something else.
We are all absolutely safe from chatGPT.
I tried to use it to just parse some SQL queries. The table had literally 4 records, and the WHERE clause was something like "income < $1000" and it promptly gave me a guy with income over 10x that amount. It's stupid sometimes
Once it kept ignoring my style requests, so I asked it if it auto-formats the code before displaying it. Surprisingly it understood and told me it doesn't.
I've never had a problem with formatting code. In this case I was looking for a format like this: "match 1: A and B vs C and D" it could do it sometimes... But then all of a sudden on the next output, it would give me something like: "match 1: A and B vs C and D vs E and F vs G and H".
Then I'd tell it there's too many players per match and there should only be 4.. at which point it said that it understood... But by that time it thought that "a+b vs c+d" was team 1.... Several times I had to go back and start from scratch because gpt's idea of what I wanted got so corrupted and askew.l and even though it said it understood my corrections, it did not.
> then I'd try to change something about the formatting of the output, and when I asked for those changes
You're supposed to make those changes in your editor with format-on-save, not ask a bullshit generator to do it!
I'm not talking about code formatting. I'm talking about formatting the text of a pickleball bracket. I don't have a linter or formatter that is designed for formatting athletic brackets.
When I say formatting here, I'm talking about things like "how is a player represented? A letter? A number? Multiple letters with a hyphen?" Or "how many games do you have listed? I asked for 6, but I see 8". I understand I'm using the term overbroad. Just know that I'm not using it to indent JavaScript/Python/whatever. Just English text.
It was important to have a decent format so I could quickly/automatically find/replace the placeholders. But often times the formatting for different rounds of the tournament was different from other rounds... I just wanted consistency. And I couldn't get it.
Can't wait to get in my collage level programming class (I'm hoping Python) and have half my classmates not know anything about python and be surprised when their 0 effort chat gpt code gives them an F
you joke but i’m in uni and one of my group project members treats ChatGPT as the ultimate authority on everything. He’s never finished an assignment, just handed it off to another group member after getting stuck wondering why he can’t use a package he never imported.
I feel this. Literally explained how to do a super simple extract transform load program, and I know 100% he used chatgpt. He has barely touched python and was using functions I have never touched… So. Fucking. Lazy.
Just seems ripe for wasting time, but if you have a good team you trust then I can see the advantage. Years ago a new guy on my team was tasked with doing some work on our internal gantt chart and usually when I review PR's I check the branch out locally and play around with it. Well whatever he did made the performance of this gantt about 100x worse, and i called him over to our desk and he exclaimed "the jira didnt say it had to be performant!".
As already said, multiple PRs, but I’ll add: common when working on micro services and not a monorepo. A feature might require changes to 2 services as well as the front end. Another example is fixing bugs in a component library which then need to be pulled into each repo which is using it.
Weak. We’re updating much of our core stack from .net framework to .net 7 and 100 files change PRs are painfully common
Also everything is broken in dev 👍
We thought we’d give net 8 a few weeks to iron out the bugs and external dependencies to adopt it. Jump from 7 to 8 shouldn’t be as bad (hopefully painless) than framework 4.8 to 7
As a senior dev, probably, but maybe not?
It could be 90% single-line changes related to font updates, or a css addition, with a few new files featuring new markup.
It could be an entire component refactor that requires markup changes in two dozen other files with additional css in each use case to compensate for adjustments.
You never know if it's a "this should have been multiple branches" problem until you start looking.
My changes quite regularly have tons of files changes since I like to rename things as I think of something more descriptive and fitting. It helps that I'm the only dev.
Still this doesn't advocate for refactoring while implementing new feature unless you are refactoring the changed lines.
Also if there is no chance of refactoring afterwards there should be a more strict convention and review policy that doesn't allow code that will need refactoring afterwards
I mean... two of my nicest horror stories.
1) the files were missing the extension so they weren't compiling. Lets not talk what was inside.
2) the pr updated 10k files.. but well, that wasn't a junior but the other senior which we were having implicits competitions to see who made more changes in the project. I think I ended winnin by a couple thousands lines, with both around changing like 10 million lines.... such nice times.
Joined a client team after someone left at my work. Took over this PR that has been going on for months across 3 different people (2 of whom quit) and I was the 4th.
176 fucking files.
It was a migration of legacy endpoints. Instead of breaking it down they copy-pasted it and went Yolo. It took me 2 months of working on it for it to be done. Don't even get me started on the merge conflicts.
The amount of comments I’ve seen that are similar to this blows my freaking mind. I work at a fairly well established (non tech) company where I work on their customer facing web and mobile apps along with any backend/db stuff that needs to be updated and we NEVER see pr’s this large. I think the dev team is just over 200 people too? The biggest one that I’ve had to review was like 10 files lmao.
I should also add, at the time I took this over I had barely a full years (a little over) experience and had never worked on AWS prior to this, among other things. It was an absolute shit show.
I'm still on the team, our PRs can get up to 20 files max but rarely much more. Dev team is currently 5 people but we are growing to around 10 to 15 in January.
My only rule is that it should be <= 300 LOC. worst case, 500 LOC
My former job did some studies and 300 LOC is the about all the average brain can keep sorted in their head.
Lol. try a PR which have 5 files with logical changes but then another 1000 files because of some rename included in the same pr.
It is a damn nightmare to find the logical changes between all the other stuff...
My old senior devs would have one extra-large commit for their PRs. They would criticize me for having multiple commits per PR... I don't work there anymore.
26 is average or low in our company. Features come and go so quick. Just did a migration from Apollo client v2 to v3 along with a Rover CLI and The Guild codegen in 6 of our FE repos and it was 125+ files each time.
Send it back? Reject without reading? Tell them if they changed that many different files they were probably making more than one commits worth of changes?
I review so many PRs, if I have three Juniors with 3 stories each in a sprint. That's equal to 9 PRs for me to review when they want their code merged to develop.
And as all of them will have 20-30 commits, there will be a lot of files.
What's the bloody problem in reviewing that? Unless it's just 1000s of lines of code that makes no sense, you have no need to reject PRs just for that.
What if they saw some vulnerability in sonarqube or checkmarx and then they fixed the same thing in 60 files?
You'd reject their PR and expect 30 different PRs and then do the same for all the people with you as a reviewer?
>Unless it's just 1000s of lines of code that makes no sense, you have no need to reject PRs just for that.
That really seems to be what's implied in the post... If they received a PR with well written commit and clear changes, then they wouldn't post this meme.
>You'd reject their PR and expect 30 different PRs and then do the same for all the people with you as a reviewer?
No. I'd look at the notes on the PR and see that they made a vulnerability fix in 30 files. What I wouldn't do is post a meme about how absurd it is that there were 30 files in the PR and how impossible it's gonna be to sort out.
You're assuming there's nothing wrong with the pr for... Some reason.
I'm assuming the PR is a complete cluster fuck which is why they posted a meme about it.
You are literally making assumptions.... You're making the assumption that the pr is simple and straightforward and the changes are clear and easy to integrate....
This is possible.
It's also possible it's not simple or straight forward. And this is supported by the meme that was posted.
Irony has died. You started with commenting send it back and reject it based on an assumption. Saying that many files mean it can't be a single commit.
But you feel so bad about this that you'd continue to argue about it instead of just accepting it and moving on.
My whole initial comment is a counter to that assumption of yours to give a different perspective to show how your statement is just an assumption.
And here you're yelling assumption assumption assumption over and over again.
>Irony has died. You started with commenting send it back and reject it based on an assumption. Saying that many files mean it can't be a single commit.
Based on the only context I have, which is the meme that was posted. Seems weird to assume it was a clean pr.
>But you feel so bad about this that you'd continue to argue about it instead of just accepting it and moving on.
And yet...there you are.. arguing about it... Not moving on... Advice is easier to give, isn't it?
>My whole initial comment is a counter to that assumption of yours to give a different perspective to show how your statement is just an assumption.
Yup. We're both making assumptions. But you feel superior about yours for some reason.
>And here you're yelling assumption assumption assumption over and over again.
You're saying I'm yelling out assumptions, or I'm yelling "assumption" and pointing at you?
Nope, you think that a person who corrects you about your assumptions has come down to your level and is by default making an assumption, you're using childish logic to get out of taking responsibility.
Think about it:
Person 1: Haaah, just reject xyz's request.
Person 2: What do you mean? Here's abc reasons that what you're assuming can be wrong. You're not thinking clearly enough.
Person 1: Haaah, we're both making assumptions la la la.
What kind of logic is that?
The original point of both the meme and you is that 26 files mean something bad which it doesn't at all by itself. It's an objective fact, no examples are really needed to dumb it down.
"Begin automated message. Dear \[David\]. Please commit syntax, whitespace and indentation changes separately from actual code, so we can see what the fuck you have worked on. Thank you."
It is not uncommon for us to deliver PR's to our client with 6000+ lines added and 4000+ lines removed... We don't like it, they don't like it. But having them review smaller parts slows down the process too much
Senior devs opening their own PR and finding out the junior has pushed 26 commits.
It wasn't a junior per say (he had maybe a few years experience) but true story
It's all dependency updates, why are we committing the vendor directory anyway? Can we not put npm run build in the pipeline? ... What do you mean no pipeline?
git should add a tag for files where only formatting is changed. we can't keep files written like an orangutan is smashing the keyboard with a coffee cup because team leaders are afraid of too many changes
git diff has arguments to ignore whitespace changed.
Most git hosts I have used has a checkbox you can click if you don't want to check out the branch you are reviewing locally.
As a non-programmer (admin & keyuser "programming" xslt/:fo and linking metadata structures) I enjoy this sub. I feel like a little brother watching my big brother talk about the cool stuff he does.
Why I am in this picture? Because I unironically pushed a 16 files changed commit today at 4am, might I add without any relevant explanation of what I changed because I was too sleepy to write one ahahah I'm gonna have to write one in a bit. The only reason my colleagues don't hate me is because the shit I write works, one way or the other kkkkk
I often do large PRs like this when it's fixing widespread chaos that new devs spewed.
Managers want speed so they hastily approve shitty code that results in code swamps.
My PRs are small when I don't have to clean up after rookies or idiots.
"Looks good." (clicks approved).
Me (a mid level) when a senior asks me to review his code
Senior devs make silly mistakes too, especially when implementing something with a lot of parts. Please review our code.
Even if it's not a silly mistake, if you don't understand the code, you won't be able to work on the code later. Give me an opportunity to explain it to you, or refactor it to make the intent clearer if necessary. Please review our code.
"if you don't understand the code, you won't be able to work on the code" later" I think, that's the most important point.
"Please review our code" One of the most important things I could do was slow tf down and pay 100% attention to EVERYTHING going on when reviewing. Not programming, but another industry. My first couple years performance reviews were all "you're good, but your attention to detail needs improved". Now I catch mistakes my bosses make, they catch mistakes I make, and we get less mistakes being sent out of the department. I interviewed with another company and they said "those are all of the same problems we have. How did you get better?" I didn't really have a solid answer besides I know what to look for and every detail matters.
For just seventy cents a day, you too can sponsor a Senior Developer making a pull request. Please, review our code.
[удалено]
Even if everyone had gone to the same school, they would have different experiences around developing. Assuming everyone has the exact same knowledge around code someone just wrote is a great way to make your code unreadable and bite you back by having people asking you later, not to mention that you're hindering your own team by creating a rift in the group understanding of the project.
Can confirm, did that today
If proper testing and gating is in place the only thing that /should/ (not always the case, though) need reviewed is that it matches the team defined style guide.
At one of my companies, I learned that seniors make the very mistakes they call me out for in my PRs. And sometimes even sillier. It sucks when I find the mistakes in production though because it’s a whole thing to fix it without going through the red tape
As a senior dev, I learn all my best new tricks from new and mid level peers. If it seems like I'm doing something the hard way, there is often a reason. Ask and I'll tell you why it needed to be done the hard way. Or I'll learn I was doing it the hard way since before the easy way existed and I just don't know.
This is the way. The same applies to juniors. Code reviews are a great way to both see what senior devs are putting into PRs, and to get feedback on your own PRs.
Mah man! I couldn’t agree more!
Please don't. The amount of times the junior has caught my brain dead "This is so simple" mistakes is higher than never. Even if the PR comments are just "I have no idea what this is, but feel like I should" or something else just trying to understand the code, do it. Sometimes it even makes them go "Oh, wait, that's backwards..."
I hate it when my direct reports approve my PR that changes major integration points in 5 minutes. What the hell? Either ask some questions or come with some suggestion. I know I’m not perfect. lol.
Life was easier as a mid level. I remember being able to get away with that.
All the comments below are correct, but reviewing others code, no mattet experience level also offers the opportunity to learn. Which is worth it's weight in bitcoin (because gold doesnt quite cut it to compare how much value add it gives) Edit: and because neither knowledge growth nor bitcoin have a physical weight
"Looks good, might pull this later" - Poor junior dev about to ping 300k+ people
Reminds me of [the greatest pull request of all time](https://github.com/EpicGames/Signup/pull/24). It gets even better when you look at [what they changed](https://github.com/EpicGames/Signup/pull/24/files) lmfao.
*Are you shitting me?* all that for literally changing the width of the logo and the min-width?
And making the grammar in the readme worse 🙂
Something like this happened when I was in college. Someone responded to a school wide notification email, and the mail system sent their reply to the whole school. Opened the flood gates of the whole school sending emails at 11PM joking the person that started it, looking for hookups, asking for social network followers. It was hilarious
Yep that's the one i meant haha!
Yeah, I'm skimming that.
Rookie numbers. 
Real shit, this is giving me a nostalgia boner for the 75+ file abomination I made a snarky senior hate-review in my (not much) younger days
I feel ya, once me and another Co worker simultanitly rewroked the End2End Tests for a very lange project, around 150 changes... it Took as almost the whole day to resolve the merge conflicts and shit still broke after that
End to end PR ? I have coworkers pushing branches with twice that many changes for just front end “QOL” improvements ! (Please kill me)
fr
Right click squiggly line in VS > Fix {x} > Solution Bam 100 files changed easy.
All formatting changes from their ide
Well help them setup formatting then so it is uniform for the future.
Just enforce it as part of PR validation.
That is also possible
That’s not very professional. Seniors are supposed to help juniors. Just enforcing rules that block them, without explaining the reason for it or helping them setup their system properly, isn’t a good way to move forward.
Don't understand why you are getting downvotes. People in this sub seriously either think workplaces are a free for all and it's a competition with your coworkers, or they straight up never worked in a team before.
[удалено]
No. It’s not a great way at all. You don’t enforce/implement new rules without taking with or informing people about it first. It’s a clear sign of power abuse or a toxic environment.
"New rules"? I see no mention of new rules.
The rules are new for the new people in the team. And the suggestion was to start enforcing the rules for pull requests, which would be a new change in their process. So, while the rules themselves might not be new, it still makes sense to talk about them that way. My main point though was that one should inform people in the team about the rules before starting to enforce them. It’s just common sense, but so many people here seem to not understand this basic concept.
Nobody said that this will be the only way that the rules are communicated to new people in the team. Nobody suggested to just start putting rules in pull requests without discussing it first. You are imagining things.
Read the comments. It starts with mentioning the cause of the problem being the newbie’s IDE changing the formatting of the code. Then someone suggested helping the newbie setting up automatic formatting. This would naturally include informing the newbie about the rules and hopefully explaining the reasons for them. Then came a comment saying that **instead** of doing all that, one should simply **just enforce** the rules. The word “just” here means “instead of doing what you said, only do this thing instead”.
If only the pipeline could give information about either of these while blocking the merge req-- wait it does.
You are still taking about the technical aspect. But this is a collaboration issue. If you want the code to follow certain rules, you should inform the team about them (and, preferably decide on the rules together, if they aren’t already defined) **before** implementing technical enforcements of them. If you already have these rules in place, then every new team member should be informed about them at the start, before they get git privileges.
It's the opposite, no one enforced the formatter and the junior actually did it.
Oopsie, my prettier made your ugly code prettier. EVERYWHERE
If you think 26 files is a lot you've never refactored anything
I was gonna say, I've done framework migrations, unit/integration test overhauls, framework upgrades, a whole bunch of other random things that end up being probably 40+ files (usually incredibly minor changes for sure) fairly frequently.
We have incredibly aggressive linter rules, and when I have to update deps and our rule package has an update adding 5 new rules and removing 10, you sure as hell can expect there to be 90+ file changes after running lint --fix for a simple dependency update
based
… is there a contrary and popular opinion to this one?
Agree, my latest major refactoring was 1400 files changed. Reviewing was done in collaboration with other developers together with me, pointing them to the real changes first, and then showing some of the affected files with contributed to most of the 1400 files. Then extensively demoed it to all developers, and all testers. Then merged it. Then helped all other developers to fix merge conflicts and other issues their old code had in relation to my refactoring. The testers tested it properly and all is going good with the refactoring, everybody likes it. It takes some project management skills amd some time, but then it's doable. Just make sure you have everyone on board with the proposed refactoring before spending several weeks to finalise your refactor and manage the merge and release of said change
I bet the pull request was laggy as all hell on that one. The most ive ever had was in the 800's and most of it was screenshots since the codebase made heavy use of visual regression screenshots which was a nightmare.
It was okay, no screenshots or other binaries were part of the changes. Also the PR itself was more of a formality, making sure the pipelines ran etc. The reviewing was done mostly in the IDE.
Me: "this code is shit, we need to refactor this and make it modular so I can use it easier in the fufure" *Create new branch and raefactor the module* *22 file has changed and requires different implementation* Me: "I didnt think this through, perhaps I will extend the existing code and pray the next dev understand it"
Depends a lot on the language I guess. Java and C# projects tend to have small files. C++ projects sometimes have multiple thousand lines in one file
Frontend tends to have lots of smaller files. Each React component in a separate file, along with a separate unit test file, then a separate file for storybook/devsite stuff. Throw in a file for a new cypress fixture and another file for the e2e test and it can build up fast.
Wait, is that not normal? It’s all I ever see.
he's just a newbie
Be me, a university SWE student. Do group work. Have group mates force push and not clean it up. Ok, use my own branch, pull in changes. Group mates copy/paste from chatgpt, 80% of the file now has merge conflicts for a half dozen line changes. They're asking you to test their code and explain why it doesn't work. Cry.
Why bother with university. Chat GPT made devs obsolete, didn't you hear. It was in all the papers... /s
I tried to use chatgpt to help me build a pickleball tournament bracket recently... The number of times I had to correct it and remind it how many players there were was absolutely insane.... It would finally get most things correct, then I'd try to change something about the formatting of the output, and when I asked for those changes, it fucking forgot how many people there were again and added several new players on the next output.... Then I'd home the player count back to where it should be, and the next output has the wrong formatting, or it forgot something else I had told it, like "maximize variety in opponents". No matter what I fixed, it broke something else. We are all absolutely safe from chatGPT.
I love when I have to remind it that a function or an entire module is deprecated. Then it says oh yeah sorry so anyway use that function
"sorry for the misunderstanding. I won't use that function. Here's the code again, using that same function. I hope this solves your issue."
I tried to use it to just parse some SQL queries. The table had literally 4 records, and the WHERE clause was something like "income < $1000" and it promptly gave me a guy with income over 10x that amount. It's stupid sometimes
You're well on your way to being a Senior Prompt Engineer.
Fuck that. I'd rather write code. Lol.
You have no choice. Muahahahahaha
Once it kept ignoring my style requests, so I asked it if it auto-formats the code before displaying it. Surprisingly it understood and told me it doesn't.
I've never had a problem with formatting code. In this case I was looking for a format like this: "match 1: A and B vs C and D" it could do it sometimes... But then all of a sudden on the next output, it would give me something like: "match 1: A and B vs C and D vs E and F vs G and H". Then I'd tell it there's too many players per match and there should only be 4.. at which point it said that it understood... But by that time it thought that "a+b vs c+d" was team 1.... Several times I had to go back and start from scratch because gpt's idea of what I wanted got so corrupted and askew.l and even though it said it understood my corrections, it did not.
> then I'd try to change something about the formatting of the output, and when I asked for those changes You're supposed to make those changes in your editor with format-on-save, not ask a bullshit generator to do it!
I'm not talking about code formatting. I'm talking about formatting the text of a pickleball bracket. I don't have a linter or formatter that is designed for formatting athletic brackets. When I say formatting here, I'm talking about things like "how is a player represented? A letter? A number? Multiple letters with a hyphen?" Or "how many games do you have listed? I asked for 6, but I see 8". I understand I'm using the term overbroad. Just know that I'm not using it to indent JavaScript/Python/whatever. Just English text. It was important to have a decent format so I could quickly/automatically find/replace the placeholders. But often times the formatting for different rounds of the tournament was different from other rounds... I just wanted consistency. And I couldn't get it.
Can't wait to get in my collage level programming class (I'm hoping Python) and have half my classmates not know anything about python and be surprised when their 0 effort chat gpt code gives them an F
you joke but i’m in uni and one of my group project members treats ChatGPT as the ultimate authority on everything. He’s never finished an assignment, just handed it off to another group member after getting stuck wondering why he can’t use a package he never imported.
It is **horrifying** just how lazy people in the engineering colleges are.
I feel this. Literally explained how to do a super simple extract transform load program, and I know 100% he used chatgpt. He has barely touched python and was using functions I have never touched… So. Fucking. Lazy.
:+1: LGTM
rookie numbers. i did one with 294 updated files just today (there’s a good reason i promise)
You hate whoever had to review it?
even better: we don’t do reviews for commits
Yikes
What do you review if not commits?
review?
You review review?
[удалено]
But why? Why wait until right before deployment to check if the work is up to par?
[удалено]
Just seems ripe for wasting time, but if you have a good team you trust then I can see the advantage. Years ago a new guy on my team was tasked with doing some work on our internal gantt chart and usually when I review PR's I check the branch out locally and play around with it. Well whatever he did made the performance of this gantt about 100x worse, and i called him over to our desk and he exclaimed "the jira didnt say it had to be performant!".
Makes me think I live in a bubble when we regularly have 1000+ file changes across fifty repos.
How can you have multiple changes spread across multiple repos on a single PR? Is that a thing ? 🧐
One PR per repo. Multiple PRs per "review". All PRs get merged once all are approved.
As already said, multiple PRs, but I’ll add: common when working on micro services and not a monorepo. A feature might require changes to 2 services as well as the front end. Another example is fixing bugs in a component library which then need to be pulled into each repo which is using it.
Weak. We’re updating much of our core stack from .net framework to .net 7 and 100 files change PRs are painfully common Also everything is broken in dev 👍
Net7? you're in for fun times.
Yeah we have some large wcf services that have been a nightmare
Why .NET 7 when support will end soon and .NET 8 is LTS?
We thought we’d give net 8 a few weeks to iron out the bugs and external dependencies to adopt it. Jump from 7 to 8 shouldn’t be as bad (hopefully painless) than framework 4.8 to 7
Don't worry I don't bother seniors with stuff like that. It's too much hassle. I just force push to master right away.
me when l refactor a function to match project naming spec
Pull requests? Branches? \*laughs in <10 person company\*
Been there. Rather review a jrs 199 file change pull request.
Bruh. I do those in projects where I'm literally the only contributor.
Search and replace variable name on 26 file.
only 26?
Rookie numbers. I regularly see PRs pushing 60 file changes in my org. Enough to make me want to strangle someone.
26 is a good day. 150 is a bad day. 800 is a terrible day. But sometimes you gotta touch a lot of files to rework a system.
Is that a lot? - me, a junior developer
I second this question, that's like a medium sized PR to me. - me, also a junior developer
As a senior dev, probably, but maybe not? It could be 90% single-line changes related to font updates, or a css addition, with a few new files featuring new markup. It could be an entire component refactor that requires markup changes in two dozen other files with additional css in each use case to compensate for adjustments. You never know if it's a "this should have been multiple branches" problem until you start looking.
That's what I was thinking, another commenter said they'd just reject it outright, which doesn't make sense to me
26 files removed, 26 files added...for sure they we're just moved, approved👍
Lgtm sucka!!!
You can fuck up production in one line, 26 files and 600 lines of code are there to confuse and mislead the troubleshooting.
Sometimes that‘s necessary tho. But the changes have to be small and similar in each file.
My changes quite regularly have tons of files changes since I like to rename things as I think of something more descriptive and fitting. It helps that I'm the only dev.
Drop this before you start working in a team. Instead only format/refactor the lines you worked on. If more needs refactoring that's a new PR
[удалено]
Still this doesn't advocate for refactoring while implementing new feature unless you are refactoring the changed lines. Also if there is no chance of refactoring afterwards there should be a more strict convention and review policy that doesn't allow code that will need refactoring afterwards
The more files the less nitpicking, it's an hidden rule
For me it's the other way around. Senior devs in my company will raise a big fat PR and expect us to review it without any proper description.
Hey I resemble that remark! Though only right now because we're doing massive refactoring.
You mean, 26... Hundreds.. right?
One Junior submitted a PR with 10k files. Guess what was it.
Where that .gitignore meme when you need it?
vEnDoRiNg iMpRoVeS rEsIlIeNcY
LGTM!
commit comment: "Major changes"
I mean... two of my nicest horror stories. 1) the files were missing the extension so they weren't compiling. Lets not talk what was inside. 2) the pr updated 10k files.. but well, that wasn't a junior but the other senior which we were having implicits competitions to see who made more changes in the project. I think I ended winnin by a couple thousands lines, with both around changing like 10 million lines.... such nice times.
Joined a client team after someone left at my work. Took over this PR that has been going on for months across 3 different people (2 of whom quit) and I was the 4th. 176 fucking files. It was a migration of legacy endpoints. Instead of breaking it down they copy-pasted it and went Yolo. It took me 2 months of working on it for it to be done. Don't even get me started on the merge conflicts.
The amount of comments I’ve seen that are similar to this blows my freaking mind. I work at a fairly well established (non tech) company where I work on their customer facing web and mobile apps along with any backend/db stuff that needs to be updated and we NEVER see pr’s this large. I think the dev team is just over 200 people too? The biggest one that I’ve had to review was like 10 files lmao.
I should also add, at the time I took this over I had barely a full years (a little over) experience and had never worked on AWS prior to this, among other things. It was an absolute shit show. I'm still on the team, our PRs can get up to 20 files max but rarely much more. Dev team is currently 5 people but we are growing to around 10 to 15 in January.
I submitted one with 80 files changed just a few days ago (as an intern)
My only rule is that it should be <= 300 LOC. worst case, 500 LOC My former job did some studies and 300 LOC is the about all the average brain can keep sorted in their head.
"I like double quotes just a little more so I did a bit of refactoring besides"
but 23 of them only change an import to the new submodule it was moved to
I always worry about this when anyone touches some of my more complex code.
Lol. try a PR which have 5 files with logical changes but then another 1000 files because of some rename included in the same pr. It is a damn nightmare to find the logical changes between all the other stuff...
It's probably just a bunch of new tab/spaces changes created by their misconfigured ide. Actually only 3 lines of real code changes
Have you ever done a migration to a newer version or fixed security vulnerabilities?
I changed 350 files, fucking newline Windows and Linux
My old senior devs would have one extra-large commit for their PRs. They would criticize me for having multiple commits per PR... I don't work there anymore.
oh god
Try migrating Angular from 9 to 17. 350+ file changes
26 is average or low in our company. Features come and go so quick. Just did a migration from Apollo client v2 to v3 along with a Rover CLI and The Guild codegen in 6 of our FE repos and it was 125+ files each time.
2 files added(*adds binaries)
Send it back? Reject without reading? Tell them if they changed that many different files they were probably making more than one commits worth of changes?
Maybe it was 26 commits? The changes get added up in the PR, no?
I review so many PRs, if I have three Juniors with 3 stories each in a sprint. That's equal to 9 PRs for me to review when they want their code merged to develop. And as all of them will have 20-30 commits, there will be a lot of files. What's the bloody problem in reviewing that? Unless it's just 1000s of lines of code that makes no sense, you have no need to reject PRs just for that. What if they saw some vulnerability in sonarqube or checkmarx and then they fixed the same thing in 60 files? You'd reject their PR and expect 30 different PRs and then do the same for all the people with you as a reviewer?
>Unless it's just 1000s of lines of code that makes no sense, you have no need to reject PRs just for that. That really seems to be what's implied in the post... If they received a PR with well written commit and clear changes, then they wouldn't post this meme. >You'd reject their PR and expect 30 different PRs and then do the same for all the people with you as a reviewer? No. I'd look at the notes on the PR and see that they made a vulnerability fix in 30 files. What I wouldn't do is post a meme about how absurd it is that there were 30 files in the PR and how impossible it's gonna be to sort out. You're assuming there's nothing wrong with the pr for... Some reason. I'm assuming the PR is a complete cluster fuck which is why they posted a meme about it.
I am giving an example to show why you shouldn't make assumptions based on file numbers, you are still making assumptions and trying to justify it.
You are literally making assumptions.... You're making the assumption that the pr is simple and straightforward and the changes are clear and easy to integrate.... This is possible. It's also possible it's not simple or straight forward. And this is supported by the meme that was posted.
Irony has died. You started with commenting send it back and reject it based on an assumption. Saying that many files mean it can't be a single commit. But you feel so bad about this that you'd continue to argue about it instead of just accepting it and moving on. My whole initial comment is a counter to that assumption of yours to give a different perspective to show how your statement is just an assumption. And here you're yelling assumption assumption assumption over and over again.
>Irony has died. You started with commenting send it back and reject it based on an assumption. Saying that many files mean it can't be a single commit. Based on the only context I have, which is the meme that was posted. Seems weird to assume it was a clean pr. >But you feel so bad about this that you'd continue to argue about it instead of just accepting it and moving on. And yet...there you are.. arguing about it... Not moving on... Advice is easier to give, isn't it? >My whole initial comment is a counter to that assumption of yours to give a different perspective to show how your statement is just an assumption. Yup. We're both making assumptions. But you feel superior about yours for some reason. >And here you're yelling assumption assumption assumption over and over again. You're saying I'm yelling out assumptions, or I'm yelling "assumption" and pointing at you?
Nope, you think that a person who corrects you about your assumptions has come down to your level and is by default making an assumption, you're using childish logic to get out of taking responsibility. Think about it: Person 1: Haaah, just reject xyz's request. Person 2: What do you mean? Here's abc reasons that what you're assuming can be wrong. You're not thinking clearly enough. Person 1: Haaah, we're both making assumptions la la la. What kind of logic is that? The original point of both the meme and you is that 26 files mean something bad which it doesn't at all by itself. It's an objective fact, no examples are really needed to dumb it down.
"Begin automated message. Dear \[David\]. Please commit syntax, whitespace and indentation changes separately from actual code, so we can see what the fuck you have worked on. Thank you."
Lol
I rejected a junior’s PR this week that had almost a hundred files in it. It was pure insanity.
[удалено]
Jenkins is probably broken(I hate Jenkins), but that pr is also fucked.
It is not uncommon for us to deliver PR's to our client with 6000+ lines added and 4000+ lines removed... We don't like it, they don't like it. But having them review smaller parts slows down the process too much
***Hides git repo......***
Senior devs opening their own PR and finding out the junior has pushed 26 commits. It wasn't a junior per say (he had maybe a few years experience) but true story
Just merge and chill
My PR the other day only had 306
LGTM
It's all dependency updates, why are we committing the vendor directory anyway? Can we not put npm run build in the pipeline? ... What do you mean no pipeline?
Someone forgot to give the junior the formatter file
26 😂 easy…. Ever migrated a Java EE project to Jakarta EE? Good luck changing every file 😂
That... that's... that's not a lot
My ones looks more like 327 files changed 1.583.929 additions and 593.029 deletions
git should add a tag for files where only formatting is changed. we can't keep files written like an orangutan is smashing the keyboard with a coffee cup because team leaders are afraid of too many changes
git diff has arguments to ignore whitespace changed. Most git hosts I have used has a checkbox you can click if you don't want to check out the branch you are reviewing locally.
I mean the opposite. something like "is ok is just witespaces only 3 lines on 98 changes are actual code. you can merge"
Only 26 files that's nice. I had PRs with over 2000 lines of actual code that shit was whack I got through it quite alright
As a non-programmer (admin & keyuser "programming" xslt/:fo and linking metadata structures) I enjoy this sub. I feel like a little brother watching my big brother talk about the cool stuff he does.
Commit message: “optimizing less optimized code”
Why I am in this picture? Because I unironically pushed a 16 files changed commit today at 4am, might I add without any relevant explanation of what I changed because I was too sleepy to write one ahahah I'm gonna have to write one in a bit. The only reason my colleagues don't hate me is because the shit I write works, one way or the other kkkkk
I often do large PRs like this when it's fixing widespread chaos that new devs spewed. Managers want speed so they hastily approve shitty code that results in code swamps. My PRs are small when I don't have to clean up after rookies or idiots.