Simple 1 day task. This idiot takes two weeks and after 7 days of hounding finally opens a pull request.

I go in to review the code. Should be a simple 10-15 line patch.

13,000 lines of code changed.

THIRTEEN THOUSAND!

"I fixed a bunch of formatting mistakes and replaced all instances of single quotes to double. Consistency is important you know."

That reminds me of when I did my first internship and tried to indent a messy 4000 lines file by hand, causing a merge conflict I didn't resolve properly.

Push was not reviewed, went through and then production was destroyed the following day.

Did he test them?

I mean it can be a minor change, or he could have replaced a bunch of pgsql script single quotes with doubles...

R.I.P. 😄

Tell that guy to split it into two PRs: one for the actual change and one for the formatting and style.
So that he won’t need to throw away days of "work".
Yes, it took him way longer to do than it should. But the time is lost and you won’t get it back. So at least see if you can use the result.
But of course let him know what he did wrong.

And now as the next task, tell him too Google what a code formatter is.

@deadlyRants suuure, if they don't already know what formatter is. If project lacks a formatter configuration, it's an opportune cause to introduce it in the project.
But if there was one, and verbose at that - hoo boy, this junior really ducked up :)

@Lensflare I have a zero tolerance for added work unless it's small.

Want to fix the indention on the method you changed, do it.

Want to do it to the whole file, no thanks.

I usually only have about 30-40 minutes a day for review(if that), not going to waste my time on a pull from someone who couldn't listen to requirements.

@deadlyRants @vintprox no formatter, this was a legacy app that was probably older than they are and had dozens of developers write it.

My rule is pretty much, your scope ends at the method you modify. Anything outside of that and you're asking for trouble.

@sariel true, such impromptu should not be allowed without filing the issue first. Seriously, some devs are so neglectic, they pretend like other collaborators don't exist.

@sariel

I think maintaining/fixing legacy SHOULD absolutely be part of any engineering position.

Without at least *some* effort towards standardization & consistency in a codebase, you will eventually start bleeding talent to other companies.

Ancient Spaghetti will increase occurrence of bugs, and can slow down or even completely block requested features.

But there MUST be communication. That is where a Scrum process comes in.

Daily standup: What did you do yesterday, what will you do today, is anything blocking you, are we going to finish the planned tasks within this 2-week sprint?

If you bring up "I'm fixing formatting for the next 7 days", and it's not part of the sprint planning, then well fuck you.

As an engineer, you put it on the backlog.

The PM must then take the technical debt tasks seriously and interweave them with "progressive work" (requested features & fixes).

But communication & planning is holy.

"Include only the changes relevant to the scope of the task. You may create a housekeeping task for other changes, but they would go separately."

@iiii have you tried reviewing code you haven't worked on in like a month... Let alone someone else's which you have no idea what it is supposed to do.

Sometimes I get pulled away immediately after finishing a project but then go back to document it for either KT or just do I can remember how to use it...

And well at that point I'm like "hmm.... How does this work again.... General architecture/flow?" What is this formula that takes up an entire method to implement.

@donuts yes. And I am trying to stay inside the scope as much as possible, because that would make the job easier in the long run for everyone that would work on the same piece later.

@iiii oh you meant housekeeping for other people's code.

Thought you also mean your own.

@donuts your own as well. Unless it's the same code you're changing in the scope of the task, do not touch it. Instead, note it as a housekeeping task and put it in the backlog.

@iiii well a lot of my tasks basically require writing a new app, hence the documentation.

And depending on when I get to it, how comprehensive and detailed it is.

That idiot probably is the only coder actually giving a shit about code style consistency.
Someone had to do it. If you see it, fix it.

Just tell him to cherrypick it into separate commits before the final merge...

@Oktokolo or they could stick to the acceptance criteria and follow direction.

If they want to fix formatting, go for it on a different task that doesn't jeopardize a deliverable.

The idiot also changed all the single quotes to double. What if they NEEDED to be that way for literals?

The more change your PR has, the higher the risk of it causing a bug.

Reduce the change, reduce the risk.

@sariel
Well, tell him to try that. Maybe you are lucky and he actually can separate maintenance from bug fixing.
Most can't. But maybe he is the rare gem in the pile of gems which are the coders enjoying doing maintainance and refactoring - of code not written by themselves.
If so, he likely is the first and last one, your team will ever see...

@Oktokolo I am one of those, but I still can't have deliverables fail because someone didn't follow instructions.

If your job was to clear a single apartment out, and you cleared all the apartments out you would be fired.

If you can't follow directions, you won't be trusted to complete anything, no matter how talented you are.