Earlier quoted context omitted.
only side-by-side view, unfortunate for my unified view taste
It has a --display=inline option now.
It works! Can't seem to find the option in GitHub README or the documentation though
Unified versus Split Diff
131–140 of 184 posts
Re: Unified versus Split Diff
#132Earlier quoted context omitted.
I go back and forth on this. On the one hand, I really like constant deep feedback. I really like the consistency benefits of having another person say “that’s too much, I find that unreadable.” On the other, I have now been at a lot of places where it was very hard to get my code reviewed, latencies of days and sometimes weeks if folks are in a particularly heinous crunchtime... And then when it does get reviewed, t…
> While I have never been at a place that did this, I have in my head the idea that the code should be an unfolding collective conversation, kind of like when folks are all collaborating on a shared Google Doc, I see that you are editing this section and I throw in a quick comment “don't forget to add XYZ” and then jump to a different part that I won't be stepping on their toes with. You just discovered pair programm…
It works well if the tasks are short and well-described. Long, complicated tasks descend into odysseys where one person zones out or gets completely lost as the other person just ends up treating them as a clumsy proxy for the IDE. Furthermore, it rarely works online.
Re: Unified versus Split Diff
#133Earlier quoted context omitted.
Everyone committing to the main branch all the time sounds like a nightmare to me, to be honest. I would probably seriously consider quitting if this was forced on me. (Or rather, I would probably just run on my private git copy, and only pull every once in a while, and ignore that the main branch always changes.) When / how do you do code review in your suggested workflow?
To be fair. I have never really heard of pushing to master as a CI technique for git. You would push to your own publicly visible branch and some script would automatically attempt to merge into a CI branch unless there's merge conflicts in which case you get notified. Then as you add things you get notified if the whole thing is breaking and get made aware of upcoming issues.
Yeah when I set up a repo, I set up two or three builds per PR:
- the code exactly as it is in that branch
- that branch merged into current main
- that branch merged into what is currently running in production (in the case that it's not what is in main e.g. tagged or branched)
This really helps to see whether it's an issue in "your" code, or whether it's a merge issue conflict causing failures.
Re: Unified versus Split Diff
#134Earlier quoted context omitted.
Automated tests aren't so much to make sure that the test you just wrote works. You are right that manual testing can do that better sometimes. Automated tests are so that the code you just implemented still works next months, when lots of other people have made unrelated changes, and weren't always aware of the interactions with other parts of the code. The automation is important, so that the tests get run, even wh…
To be clear, I'm not against automation. What we do for a living is write code, so when I say "automation is just more possibly buggy code", I'm not opposed writing more code (though I am opposed to overengineering). My point is that I don't place all of my faith in automated tests, and I'm not opposed to rolling up my sleeves, getting my hands dirty, and doing manual labor. Manual labor can be tedious, though, and t…
The problem starts when people do neither: only superficial test suites, not running a few test cases locally, and only start using the debugger when they need help from other people with neither the time nor patience to just stare at the code and argue about things without actually trying to get answers. For example, by provoking the problem and turning on the freaking debugger.
I fear half of my team can't run the application locally. To be honest, there are lots of moving parts in the codebase, but it's not only that.
Re: Unified versus Split Diff
#135This is an huge issue for us in a very big and complex codebase with a lot of engineers working on it. Code review is hard because the diff always looks reasonable, the tests always pass and all the basic stuff are always checked. However, it happens often that, even if the changes looks reasonable they are wrong. The whole architecture may drift after one bad change that looks reasonable. As always this is not stric…
> the tests always pass > it happens often that, even if the changes looks reasonable they are wrong I’m having trouble reconciling these two statements. When I review code, I’m rarely looking for bugs. Instead I’m looking for tests that would catch those bugs.
Do you have tests that verify that an abstraction hasn't sprung a leak?
Re: Unified versus Split Diff
#136This is an huge issue for us in a very big and complex codebase with a lot of engineers working on it. Code review is hard because the diff always looks reasonable, the tests always pass and all the basic stuff are always checked. However, it happens often that, even if the changes looks reasonable they are wrong. The whole architecture may drift after one bad change that looks reasonable. As always this is not stric…
A somewhat minor nitpick, the word huge begins with a consonant sound, not a vowel sound. It would be correct to write "a huge issue" not "an huge issue".
depends if you pronounce it 'uge or not ;P
(which is why you see some people write an history, because of the 'istory pronunciation)
Re: Unified versus Split Diff
#137Earlier quoted context omitted.
One area where vscode+gitlens plugin beats IntelliJ, the plugin adds in fully featured editable RHS to diff view
IntelliJ's spli diff view is a full editor with code completion, formatting, symbol lookup, and everything.
Yeah you're right, I have no idea what i was thinking of, intellij's supported this forever.
Re: Unified versus Split Diff
#138For code review, the top one works for me.
In a code review, the code on the right is where the focus is: is that correct?
If that change is merged, the right side version is what the code will be; the left side becomes a historic artifact indicating what the code was.
I glance on the left to understand what is changing: are some aspects changing that are not intended, and such.
There arise situations when a diff is total garbage, because code has moved around while being changed and whatnot. Sometimes unrelated code is diffed together. In the split diff you can still see the new code how it should be, but it's hard to track the changes.
In git, you can influence the diff algorithm to get a different diff, e.g. "git diff --diff-algorithm=minimal", documented as "spend extra time to make sure the smallest possible diff is produced.". This might be similar to GNU diff's --minimal option.
Re: Unified versus Split Diff
#139> For a large change, I don’t want to do a “diff review”, I want to do a proper code review of a codebase at a particular instant in time, paying specific attention to the recently changed areas, obviously every team and ticket is different, but IMO unless the person doing the review is some sort of principal engineer mostly responsible for the code at large, this does not align with I would personally consider a cod…
> In my book a general code review is simple sanity check by a second pair of eyes, which can result in suggestions to use different API or use an API slightly differently. This is an impoverished view of code review. Code review is a principal mechanism for reducing individual code ownership, for propagating conventions, and for skill transfer. A good code review starts with a good PR: one that outlines what its goa…
This is why you should insist on a changes being reviewed by at least two pairs of eyes. Someone responsible for that area of the code might raise those kinds of objections. Someone who is more of an outsider can focus on other problems.
E.g. I'm kind of a human compiler; I can say things like, you have undefined behavior here, in a completely unfamiliar code base where the maintainer of that might miss the issue, too busy pointing out that it doesn't respect architectural conventions, and is not tested.
Re: Unified versus Split Diff
#140Earlier quoted context omitted.
> While I have never been at a place that did this, I have in my head the idea that the code should be an unfolding collective conversation, kind of like when folks are all collaborating on a shared Google Doc, I see that you are editing this section and I throw in a quick comment “don't forget to add XYZ” and then jump to a different part that I won't be stepping on their toes with. You just discovered pair programm…
With the right pair it is unbelievable how much productivity can be unleashed. I concur, it's much easier to stay focused on the task at hand with someone actively working on the same thing. The reason why this didn't catch on is that it's almost like a Mick and Keith sort of relationship. You can't just take any two musicians and throw them together and get the Rolling Stones and the same thing applies to pair progr…
> You can't just take any two musicians and throw them together and get the Rolling Stones and the same thing applies to pair programming.
I don't see that effective teamwork should necessarily depend on compatible personalities. I think the better comparison would be to surgery teams, or pilot crews, rather than art.