Review Please

Doesn't matter. One concern per PR. Refactoring and tech debt are separate concerns.

CC BY-NC-SA 4.0

A tiny bit of value, but definitely not worth the pain and effort. It wasn’t exactly any technical debt that hindered our development.

We had other places with way more pressing technical debt that could’ve been focused on instead.

I hear you, but they should MVP the ticket, close it, then concisely collar the PM/lead and say "I know how to make this better and am hungry to do it. Let me address some tech debt next sprint. I got this and will keep it timeboxxed. I'll also ensure my changes pass QA before coming to you"

Real men test in prod

Reaaal men of geeenius

[Open]

"LTGM!"

• Last update a year ago

Better than "rejected - git gud"? :-P

Or auto rejected when the format doesn't fit.

Some diff tools don't handle indentation by default.

So if you add a wrapper, it counts everything inside it as "changed"

You can do that? How?

Joke's* on you

(Short for "The joke is on you".)

Make those changes in an own commit, or keep it to files you already have to touch.

When using git and are working on a feature, and suddenly want to work on something else, you can use git stash so git remembers your changes and is able to restore them when you are done. There is also git add -p this allows you to stage only certain lines of a file, this allows you to keep commits to a single feature if you already did another change that you didn't commit (this is kind of error prone, since you have to make sure that the commit includes exactly the things that you want it to include, so this solution should be avoided). But the easiest way is when you get the feeling that you have completed a certain task towards your goal and that you can move on to another task, to commit. But if you fail you can also change the history in git, so if you haven't pushed yet, you can move the commits around or, if you really need to, edit past commits and break them into multiple.

I tell my young developers - the primary goal in software engineering is maintainability. Code reuse, encapsulation, abstraction, and myriad other concepts all contribute to ease of maintaining source code over the long term. Maintainability allows for easier, predictable feature addition and removal, with fewer changes, and by different people. You're also a different person than the one you were months or years ago when it comes to software.

Do you have ADHD?

Very relatable. Especially when it's less effort to make the change than it is to try and ignore it. But it's understandably harder for those who are reviewing your work.

I'm the same way. Chasing code changes through the codebase then fighting with an edit rebase stack trying to explode them into less-interlocked changes.

It doesn't always work, sometimes I am just committing a giant blob of changes at work on my project I near-solo maintain 💀

It could be!

But part of working as a professional on a team is communicating and achieving consensus. Just trying to make a change like that out of the blue is poor form.

Also consider the opportunity cost: we had planned on getting XYZ done this week, and instead he spent a few hours on this and dragged a few people into a "do we want to change to poetry right now?" conversation

I think it's definitely a thing most people grow out of when they gain experience.

My boss told me about how when he was new he rewrote a whole chunk of the front end. His boss gave him a talking to about how you can't just go and do that when you're working with a team.

At an old job I just opened a PR to apply a code formatter to an internal project I wasn't even a routine contributor to. PR was rejected and I learned a valuable lesson about talking and getting buy-in before making sweeping changes.

Bugs are all gone, boss

😱

Stuffing multiple tasks into one PR is often bad.

• It’s harder to review. As a reviewer it’s difficult to know which code change is related to which task.
• It’s harder to verify. Did you really test every change you made?
• You might end up with a “hostage” situation. There might be a few code changes in the PR that looks good and is really wanted, but other code changes in the same PR of lower quality. As a reviewer, should you just let these lower quality code changes slide so you can bring in the code change you really want? Probably not, but you’re going to let it slide either way.

Right but it's pretty rare that a tiny PR actually accomplishes a valuable user story.

So my point is just that lines of code is mostly irrelevant as long as it's organized well and does no more than necessary to accomplish the agreed upon goal.

also iirc gitlab does offer something like this as a feature now with "merge trains" (though i've never really used it, usualy just go for the feature branch out of habit x) )

Making PRs against a feature branch still has the same problem.

Say you're working on a major new feature, like adding a new log in flow. It's a good idea to split it into many small changes: create initial log in form, implement user sessions, add SSO support, add "remember me" functionality, etc.

Those changes will likely depend on each other, for example adding the "remember me" checkbox requires the form to actually be built, and you probably don't want to wait for the reviewers to review one change before continuing your work. This means you naturally end up with PRs that depend on each other in a chain.

Stacked PRs (or stacked diffs, stacked MRs, whatever your company calls it) means that:

1. The code review tool lets you specify dependencies between the PRs, for example the "remember me" PR depends on the initial login form implementation
2. It shows the dependencies visually in the UI, like a chain or tree
3. Changes can't be landed until the PRs they depend on have been reviewed
4. There's a way to land an entire stack
5. When you review a PR later in the stack, it doesn't show any of the changes that were made earlier in the stack. Each PR focuses just on the changes in that part.
6. For each PR, the build steps and linters run for all the changes in the stack up until that point. It helps detect if incompatible changes are made earlier in the stack.

Making all your commits directly to a branch then creating a PR for the whole branch is similar, but reviews are a nightmare since it's only a single review for the entire branch, which can be thousands of lines of code.

At my workplace, we use feature flags (essentially if statements that can be toggled on or off) rather than feature branches. Everyone works directly from the main branch. We use continuous deployment which means landed code is quickly pushed to production. New features are hidden behind a feature flag until they're ready to roll out.

you should meet my coworker. this is one week worth of work. and he still only commit once a week.

The only way I see that is a dependency update is if you're versioning your node_modules or <insert-folder-here> which is generally a no-no

Or maybe their IDE had a different auto indent config and they saved it all, then committed it all without checking the diff or the status.