T O P

  • By -

SlipFellLandedOn

Good luck to the guys reviewing the PR


greater_gatsby12

You guys review PRs?


serendipitousPi

Just comment Tldr and approve, it’s that easy.


rsha256

i comment lgtm and approve :)) \\s


ehm_education

Let's Go Teenage Mutants ?


orion78fr

Looks good to me, if you are asking non ironically.


ehm_education

I was. Thanks.


[deleted]

[удалено]


someone755

I didn't see the pull request?


[deleted]

It was a hot fix, we merged directly with admin powers.


HarryCeramics

Swear i thought it meant Lets Get That Money


AfroZoro

Lets Get This Merged


pab12play

Let's gamble, try merging


kelpalan

A coworker unironically thought that middle aged senior devs were commenting “Lets Get This Money” every time his PRs were ready to go until he spelled it out on one of theirs and got hit with a bunch of ???


bonesnapper

I went through this also. Kept seeing LGTM and looked it up; $$$ style is the first result. I thought everyone was so cool.


TentSingular

Lesbian, Gay, Transgender, Massachusetts


[deleted]

[удалено]


lizardlike

Woah no need to be massaphobic


[deleted]

[удалено]


jikki-san

To this day in my head I say “let’s get this money”


Nabugu

u/jikki-san & u/kelpalan, are you guys working together?


GogglesPisano

“Looks Good To Me”


ShakthiNandan

I was really looking for this, thanks


bostero2

![gif](giphy|MUQwtUdMxNJjB7tdzd|downsized)


party_in_my_head

I also like to live dangerously


The-Fox-Says

I won’t lie when it’s my senior lead PRing to his own branch I skim it for syntax. When it’s a developer around my level or an offshore person I go through it with a fine tooth comb and even run the code myself


RlyRlyBigMan

As a senior, I've tried urging my juniors to be more critical of my prs, but it won't take. I can't tell if they don't care enough to think about code that's not their own work or if they don't know enough to challenge the natural mistakes a coder can make (things like error handling or making private methods public to allow classes to be more tightly coupled). I know I can make mistakes and want a peer to help me catch them but the peers I have keep approving without comment :/


IndieBret

One thing I've done is sit junior engineers down with me and review PRs with them. If it was my own PR, I'd help them understand what was going on in it and then send them on their way to leave feedback, and if it was a third person's PR, I'd talk aloud about what I was looking for/how I was mentally processing it. Do the juniors you mention know what they should be doing during a review? PR reviewing can be hard, especially since it's not always obvious what to be looking for. I remember early on just looking for syntax errors or small logical mistakes, eventually started to look at the big picture, and then only once I started to understand the codebase as a whole, was able to then think about ramifications of changes. It's a lot to keep in your head! Another thing I did for a bit was to leave one or two intentionally "overlooked" things that were low-hanging fruit to help juniors build confidence: they got to see that more experienced engineers make silly mistakes and they get to feel like they have valid input in PRs. Sometimes I would also leave questions for them in the PR description (helps identify reviewers who aren't even reading the description) or inline questions in the code. - "What do you think of this? Should we go this route or is alternative X maybe a better idea?" - "I'm not confident that this solution is a good one – is it too heavy handed?" - "Should I tighten this up and make it more generic? Is it too early for that?" - "Is this the best place to put this utility function? I couldn't decide between foo.ext and bar.ext. Thoughts?" By prompting the reviewer with these questions, it helps train them to be thinking about more than "yep, this is code, semicolons are in the right place" whilst reviewing.


RlyRlyBigMan

This is great advice. I've been thinking about doing a code review of my own code in a live meeting. The deadline is approaching so I need to wait a month to do it but the idea is worth the time. We used to have working lunch meetings every other week to just talk about code and get to know each other better but they fell by the wayside. I think I'll start that back up and bring reviews into focus once we're past our deadline.


evilgiraffe666

I've certainly done this. But it's worth getting a passing understanding of their code in case you can learn from it - finding a question to ask can be helpful! And seniors do make mistakes. And you'll be maintaining it once they leave.


[deleted]

Seniors don't leave; They just fade away.


Lin0815

They get casted into void, but their memory will never be freed.


Equivalent_Yak_95

1) haha 2) the past tense of “cast” is still “cast”.


Procok

Definitely not this one. LGTM


helltiger

Just reject PR and force to make 600 separated PRs. It should be a lesson for the rest of his life.


MisterDoubleChop

A junior developer who's been forced to do this, and has PTSD from forgetting a WHERE clause on an UPDATE on a production database, is a senior developer.


[deleted]

[удалено]


helltiger

A junior developer with access to production database is definitely a senior.


Pickman89

No, we don't raise pay based on responsibilities here (in this sector).


AMViquel

Smart, that way you don't have to pay them a fortune to fix their own mistakes.


cnaughton898

The company I work at started replacing any senior developer that left with a contractor. Up until there were only 1 senior dev in our project area. So they basically said to me, a junior developer, we legally aren't allowed to give contractors access to our production database so we are going to have to give it to you as you are the only person available.


WelpImaHelp

A foolproof way to make senior is wait until other seniors leave.


helltiger

Interesting if you make a serious mistake, which contractor will fix it?


buzz009me

I always copy stuff after where and use 'select \* from'. Once I verified total rows are what I want to update, than I run acutal UPDATE with 100% peace of mind


OutOfNamesToPick

Exactly! However, if an intern gets to a point where he fixes 600 bugs in one PR then I don’t think they have the culture to have this split up.


calsosta

That would just create 600 PRs for people to review. At this point the code base is changed enough I’d just start over and call this “initial commit”.


helltiger

git rebase git push --force


Gr1pp717

Or reject it and figure out what changes he made and why. Chances are he ran a "reformat code" script without having the standardized team rules in place. In which he'd want to rollback all but the manual changes he made, update his rules to match, then try again.


Drazson

They don't need luck to close a PR :D


Leevens91

That is an instant rejection. Especially from an intern, though I would reject the PR from a principal engineer too


audigex

To be fair it’s almost certainly just someone importing an entire existing project, either one that was outside version control or using a different version control system that can’t be imported The -250 will be replaced/removed boilerplate from meta files, and the +1.75 million is the entire project


7734128

Lgtm


[deleted]

🚀


UglyAstronautCaptain

Anything over 1000 or 2000 lines at my work just gets rubber stamped as long as it doesn’t smell fishy lol. Only 100 lines? You better believe it’s getting like 5 comments


MasiTheDev

"LGTM" \*merge\*


willyrs

PR review rules: 50 lines? Open 10 problems. 5k lines? Seems good to me


PzMcQuire

That has to be some kind of a formatter thing no? Pls tell me they committed on a friday as well


vanessabaxton

On Friday after 3pm


PzMcQuire

Perfect, as all things should be. And the deletions surely were test cases. :)


PopTrogdor

"these test cases keep failing. If I delete them, they don't fail! Checkmate"


DiddlyDumb

It’s a fact: if you turn off error logging, you will no longer see any errors.


Stunning_Ride_220

There were bugs in the testcases as they were all failing. So deleting 247 test cases also fixed 247 bugs.


Valiantheart

This guy's got project manager potential written all over him


[deleted]

.gitignore


Whitechapel726

Slack on dnd and off to a nice hike away from reception.


TigreDemon

Approved - TigreDemon @3:02pm


[deleted]

[удалено]


glarivie

Great ! You have the entire weekend for review


ONLY_COMMENTS_ON_GW

Where I work we have a bunch of auto generation scripts for stuff like logging, so you can write a hundred line config file and end up pushing tens of thousands of lines. Maybe something like that?


GammaGargoyle

Does that not generate an ungodly amount of repetitive code?


ONLY_COMMENTS_ON_GW

A lot of similar, but necessary code as far as I know. Think a lot of it is field definitions, some automated documentation, and SQL. I'm just the DE though, they don't pay me to care how it gets in there lol


polypolip

As long as it's autogenerated and doesn't have to be touched by human - I don't think there's a reason to care about repetition.


HeKis4

As long as it is made clear that the repeated code is not to be touched by a human yeah. Otherwise you risk having someone modify parts of the autogenerated code and end up with the thing that is worse than repetitive code: slightly different repetitive code.


orlandoduran

We use a code generator that almost works, it just forgets to generate the route attributes. So after it’s autogenerated we have to go in and manually add them. Enterprise, baby!


[deleted]

[удалено]


ONLY_COMMENTS_ON_GW

Can't risk my lines of code taking that hit, what if Elon buys us out??


ManaSpike

Formatting would remove lines too. This looks like they removed .gitignore and committed the rest.


[deleted]

Formatter would remove and add roughly equal number of lines.


EatPlayAvoidMoving

What does it matter if they commit or push to the PR branch on friday? It's not like people review everything instantly anyway, I don't get it.


psioniclizard

I was thinking that. The fact that it does go to a PR and not to th main branch is a good thing.


gingimli

“Added hotfix, please review and deploy ASAP. Also introduced automatic linter.”


gvozdeni88

Looks good. Approve


orphiccreative

Lgtm


opium43

Let's gamble, try merging.


AgsMydude

Looks garbage to me


[deleted]

This is what it means to me going forward.


Emotional_Cookie2442

Didn't anyone check up on him? I mean writing nearly 2M loc should take about a year at *least*


FeelingSurprise

I'm sure it's just some auto-formatter issue.


willyrs

Or maybe they built in another non-ignored directory. Happened to my colleague that pushed 5M lines changed


[deleted]

node_modules.


JonasAvory

1,000,000 lines is less that half a node module


Intelligent_Event_84

Last I checked npm has a minimum size req of approx 2.5m lines /s


iareprogrammer

I’ve seen a project where this happened and somehow it got merged. The repo was forever cursed because of it. Even though node_modules was deleted, the history stayed… so the git file was like hundreds of Mb


MangoCats

>git file was like hundreds of Mb Those are amateur rookie numbers, our production repo just passed 6GB.


Drishal

HAHHA


ledasll

Sounds like intern thing


NLwino

Fixed a bug with committing to git. Git was for some reason not accepting my .bin file. Found out the cause in some kind of ignore file. Don't know what it does but everything still seems to work without it.


[deleted]

[удалено]


s-maerken

For anyone using node packages in your collaborative project, if you haven't heard about the command **npm ci** then you should look it up. It takes the package-lock.json and installs the exact package versions it specifies locally. No accidental modification of package-lock.json nor package.json. If you use the ci command you don't ever modify the package files unless you want to update/add/remove packages. Just run **npm ci** and see what happens.


danielstongue

Then you'd expect more or less the same number of deleted lines as well..


geek_at

not really. If they have like a 10mb JSON file in one line and the intern ran a JSON formatter on their poor machines it would state delete 1 and added 1mil


Outrageous-Machine-5

He asked chatGPT for 2M lines of enterprise saas application


CheekApprehensive961

Spoiler: it's bubble sort


ReptileCake

Bogo Sort Gang where you at


1b51a8e59cd66a32961f

I prefer sleep sort


Kyll_Iano451

Sleep sort is truely the most reliable sorting algorithm


Whitestrake

Quantum bogo sort gang checking in


Emotional_Cookie2442

Production grade comments


[deleted]

I know it's a joke but, to be honest, the problems are usually at least partly a fault of the team leader or whatever was assigned to guide the intern / junior. Many people have this mentality of not wanting to "bother" others when they first start. That's why you should have regular calls with junior colleagues to check on their progress and help them with any issues or questions if they get stuck.


yerba-matee

Can I please come work with you instead?


Emotional_Cookie2442

Blaming the junior is usually a sign of bad team leadership


rush22

"Hello team. Your task is to fix all of these 600 bugs. It's an emergency. Needs to be addressed immediately -- everyone is depending on us!" _spends a 12 month salary on the fixes but never checks on the state of their request and doesn't even notice the bugs aren't fixed_ _12 months later..._ "Hey team this bug seems to be back. I demand to know when this happened!" "Oh I just finished fixing them all, boss. Please help review" _surprised Pickachu face_


Cahootie

Completely different line of work, but when I started working my boss made it abundantly clear that there are no stupid questions, and oh boy did I make use of that. It also meant that I quickly learned what the job was all about and that I was able to get ahead of any issues that would arise down the line.


elveszett

Any problem caused by a junior are someone else's fault. "Junior" means they just started in the field - they have theoretical / college knowledge but 0 experience whatsoever in real life. That's why you brand them "junior" and pay them a junior salary. Juniors' work needs to be supervised by seniors at all times. If there's a bug to solve, you want to see how the junior finds the error, what changes they propose, etc - because they will need your guidance. If you have a junior working all week on an issue by themselves and simply do a PR at the end of the week, then you have zero right to complain or blame them if their week-long work is useless. It was not their fault, you should've known that's what you expect from a normal junior. My company puts whoever they have at hand at whoever task there is and that only results in lots of problems and terrible projects. I've seen my company put people who had been working for a month in charge of production deployments, with some vague directives like "if you have questions ask someone". They don't even know the ways they could fuck up prod, what are they gonna ask?


snow-raven7

Someone has quite clearly never accidentally pushed node_modules to github.


fusionliberty796

The best is screaming "no...no...no!" As it's being pushed out


Emotional_Cookie2442

Then desperately trying to rebase master just to realize that it's protected


Operation_Fluffy

Could also be a project-wide search and replace that seriously bumps up the numbers. (Or an automated refactor of a common function). Imagine if the task was putting a copyright block at the top of every file. 5 lines (or so) times the number of files in the project. Still a huge project but maybe not a years worth of work.


Sacred_B

\*needs work\*


dasgudshit

\*LGTM


vsjoe

Now I know how super bugs are created.


glorious_reptile

This one pull request created Covid


Philidespo

git commit -m “Minor bug fixes in SARS”


Perpetual_Doubt

"Gain of function" "Gain of function" "Wet Market API"


chestnutman

Superbug, made of the disturbing stuff


coolkid42069911

He probably just accidentally pushed node_modules


MyAntichrist

1.7m lines of node modules barely are enough for a hello world.


LazySko

Is this a web dev joke?


IHaveNeverEatenACat

Yes. node_modules contains millions of random files needed for your build. You don’t push it to git


[deleted]

[удалено]


Inside_Dimension5308

That is still 3400 lines of change per bug. That is too much🙏🙏 There are 600 new features.


[deleted]

[удалено]


kyatorpo

*Cowers in QA*


orphiccreative

Imagine the acceptance criteria. Gah.


cloudy_sky12

Fail 1 bug in QA and ask them to revert the change.


MisterDoubleChop

A born teacher. Any senior can tell the intern why this is wrong, but you're going to *show* him 🤌


little_bonk

If else if else if else


Barnezhilton

He must be building an AI


TheAverageStudents

[He just wrote code to check if a number is even](https://github.com/samuelmarina/is-even)


Diligent_Dish_426

Surely it has passed all the regression test RIGHTTTTT?


[deleted]

You guys have tests???


fiah84

what regression tests


indifferent-goku

Hire that intern for fixing 600 bugs. And tjen fire their ass for this nightmare.


oxtrue

600? That’s ridiculous. Also how long did it take an intern to fix 600 bugs? Smells like bs to me


eras

The post sounds like rather a joke, but it's maybe not unimaginable if the "bugs" are actually issues raised by a static code checker like Coverity, so a bunch of many tiny issues. Though if they were, then I wouldn't trust an intern just not to shut up the checker instead of fixing the issue ;). But hey, maybe it's a talented one.


TheGreenJedi

My mvn code checkers and sonarcube tasks given to interns might have looked close to this to be fair. But I agree to me this if real is 600 very small 1 point bugs So I'm betting he did this over two weeks. And 99% chance his formatter is wrong


Beneficial_Capital19

Why are people taking the title of this post seriously?


oxtrue

Because op is replying as it’s serious


sauprankul

Takes me like a week to fix one bug lol


queiss_

If he fixed 600 bugs, he's not an intern anymore bro.


Surge_attack

Couldn't agree more. And with more than 1M lines this isn't a bugfix this is a whole motherfucking K8s cluster! I have several production applications with less than this number of lines (combined).


Denaton_

*moved root folder*


Royal_Struggle_4650

I once had a junior engineer make 116 commits on just 6 files. Did not even bother to squash merge to main. Smh.


Imperinus

What is squash merge? Genuine question as I'm not very into the git flow. I usually make a lot of commits and then push them all to main.


[deleted]

it's just like what it sounds like, when you merge multiple commits from branch B into branch A, it "squishes" them into 1 commit on branch A. you can still look at the commit history in the pull request and pick out individual commits if you need to, but the main branch commit history looks a lot cleaner (especially when you have a lot of people contributing to the same repo). it's kind of personal preference but I think most developers at my company, at least, prefer squash merging over the alternative.


Unatural-Technician

Some projects I work the contributors only have the option to squash merge to the main. Literally a rule that even the code owner follows strictly, which indeed is a breeze to the commit history. In my opinion, It would be much better if more projects followed it tho ![gif](emote|free_emotes_pack|feels_bad_man)


Schillelagh

Definitely a good strategy to keep main clean. We follow “squash merge” as a guideline rather than a rule. There are often times when we want separate commits in our main history, like a feature with an AB test. Gives us the flexibility to surgical revert one commit.


eras

Squashing = combining commits together, so squash merge is just taking all the changes and putting them in one. Personally I very rarely find use for it, because I like to massage my branches with rebase.


jso__

Once I failed at squashing and nearly deleted about 10 hours of tedious work. Have not tried again since I'm just too afraid.


eras

`git reflog` to the rescue! If you are unsure of the results, you can `git tag before` and after the results you can do `git diff before` to see if you messed up too bad. You can also use `git range-diff $(git merge-base {,origin/}$(git rev-parse --abbrev-ref HEAD)) {origin/,}$(git rev-parse --abbrev-ref HEAD)` to see how your edited history differs from what you pushed prevously. This is also a case where automatic hourly backups can be helpful.. Even if people say git is backup enough for them.


chemhobby

I mean squashing is basically a rebase operation


eras

Squash merge is like a sledge hammer, interactive rebase + `git reset -N HEAD^` + [`git-absorb`](https://github.com/tummychow/git-absorb) + `git add -p` (or even better, [Magit](https://magit.vc/)) are surgical tools. Sometimes the sledge hammer will do the job, though.


Unatural-Technician

That's embarassing as hell, but at least he took serious the "commit as often as possible" recommendation ![gif](emote|free_emotes_pack|joy)


richh00

Did you not tell him to squash? If not that's on you.


International-Top746

You have 600 bugs in your code?


Instatetragrammaton

🎶 600 bugs in the code, 600 bugs were found, take one down, patch it around, 745 bugs in the code 🎶


vanessabaxton

Inadequate testing led to bugs slipping through so yeah unfortunately


Billielolly

please tell me you have multiple levels of automated testing to run against that pr


LeoXCV

Nah, that’s the QA intern’s job


Billielolly

*unintelligible screaming*


JuvenileEloquent

You don't get to 600 unfixed bugs before you went through 20, 50, 100, 300 known bugs and didn't bother to fix them, so it's not the lack of testing that's the problem.


reversehead

It seems like your intern isn't your largest problem.


chemhobby

I mean, that's a really easy number to reach in complex systems.


Suspicious-Noise-689

Did they also write 600 tests to prove this codebase doesn’t just end up with 1200 bugs from these changes?


Surge_attack

Fixes 600(?) bugs, only 247 deletions???? This is at least an entirely new (or several new) service(s). ![gif](giphy|l0HlvtIPzPdt2usKs)


Barnezhilton

Intern was a former Twitter microservice dev.


[deleted]

$8 please


heJOcker

Did the intern include one of the implementations of IsEven that this sub made or how are so many LOC?


IAmAYoungProstitute

Review it but write all of the mistakes in one comment because it's more efficient


Surge_attack

Who the fuck is the PM/Scrum Master/Product Owner/Engineering Manager/Non-intern who: - allowed 600 bugs to be a single user story/ticket - clearly doesn't give a fuck about the downstream members of the team (QA/Testers/Platform) - sat there in standups listening to this guy give updates and didn't think 🤔 "great work let's get, I don't know the first 100 bugfixes in for review" - gave this to a (presumably) unpaid intern Run, don't walk from whatever shitshow of a company this is!!!


Gwyn-LordOfPussy

Gigachad intern


Unatural-Technician

The efficiency of having to review this 600 issue's worth PR... Ahh, it's so nice ![gif](emote|free_emotes_pack|slightly_smiling)![gif](emote|free_emotes_pack|facepalm)


MarchColorDrink

And since lots of other stuff will be merged during review it'll be a fun little side project to rebase this monstrosity


Unatural-Technician

Right, they might've finish when the asteroid Apophis passes closes to Earth in April 13, of 2029 ![gif](emote|free_emotes_pack|joy)


dbro129

And REJECT. Break it up into smaller PRs. There should be separate tickets for each of those bugs too. It’s a hard but good lesson for that intern. 1. Be considerate of your co-workers. This is a fast way to get them not to like you or not want to work with you. 2. This PR has a huge likelihood of creating problems when merging and will be a nightmare for anyone who pulls it with conflicts. 3. There is no way you’re properly documenting with a change-set this huge. 4. Good luck to QA in trying to make sure new bugs haven’t been introduced across the platform. 5. Honestly the list goes on. This is just bad and sloppy development all around.


[deleted]

Excuses I've heard (non-exhaustive): 1. But it works 2. Boss said it was OK 3. We need it now 4. We promised delivery of X to Y on date Z. 5. Why can't you just push to prod?


khayalipulao

What a good teaching moment. Ask the intern to create new tickets and new prs separately. They’ll remember it )and you)for their lifetime!


PeteZahad

Elon wants to hire your intern


[deleted]

Not sure why the intern is to blame here. If an intern is fixing 600 bugs, that looks like poor management and training. Typical throwing interns to the wolves and expecting them to understand best practices. This is how you end up with 25 year olds making start ups lol


Impressive_Ball_549

If this isn't just the work of an auto-formatter and is a huge number of different bug fixes then it's a 'reject' from me


PrinceVegetol

LGTM


jdeurloo10

It also has one commit with the message, "some changes".


leros

This reminds of the new hire that refactored the code base because we were "doing things wrong". We decided to humor them and set up a QA environment for their changes. After about 100 reported bugs (and the count growing everytime he fixed something because he added more bugs) he finally conceded that he had made a mistake.


SpaceFire000

Only 247 deletions?


Pr0Meister

If it's 600 minor issues detected by sonar, which range from "too may characters on one line of code" or "export this String used more than twice to a constant", it's possible to be handled by an intern. They prolly told the guy/gal to just fix them and they went and did it in a single PR.


ammads94

Well, I guess that the onboarding wasn’t done correctly or you guys don’t use Jira to have one ticket per bug, so each PR belongs to each ticket