Comment by lifeisstillgood | Hacker News Reader

Comment by lifeisstillgood | Hacker News Reader

lifeisstillgood

I am slowly convinced that comments on PRs are like comments on blog posts or youtube videos. Ephemeral, irrelevant and ineffective. If you really want to "reply", put up your own blog post or a "reacts" video. Same for code. unless it's simple typo fixes or improvements, deeper fixes come from writing code samples yourself.

I recently commented on a juniors code, and put in about four lines of code showing how I would improve the performance- but to do so I of course had to bring up a REPL and put the code in and run it and had more or less done a PR.

Make it faster and easier to inject and piece of code into the PR flow (branch pu?), make the whole code base simple to run from any point in memory, fully testrigged, so "just those highlighted lines" can be run and everyone can see.

What we are doing with this OP approach is making it easier for a human brain to imagine what the runtime will be like. That's the wrong approach - make it easier to have the runtime run over the chnaged lines of code and let people step through at PR time, and then make their changes alongside.

Or just leave a comment.

But we know what the rules are on comments on the internet

One practice I’ve gotten into with juniors is checking out their PR, branching it, PR-ing to their PR, and then having the discussion about what I want them to change there. This is good because it lets us isolate certain issues (and truly resolve them) rather than comments getting pulverized by lots of little commits, only to have someone else come in and say “LGTM!” and merge it with unresolved issues.

Another obvious way to fix OP’s issue is to request developers to break their PR’s down into smaller chunks, pair on the parts that need help, and slowly merge things from there. YMMV of course - but any 1000 line PR is going to be a headache regardless of what methodology you’re using to review code.

I like this approach too, but almost no one understands it (i.e. the idea it's possible is foreign) and UI support for it is non-existent in all the major products.

It would be wonderful if we could ditch "change these lines" type comments in favour of just letting merge requests with the changes be easily surfaced.

Yeah, it really wouldn’t be that hard to implement though. I think I’ll take a stab at something like this using GitHub cli tomorrow or Monday

I might be wrong but it sounds like you don't know about the GitHub PR feature where you select the lines, and GitHub allows you to edit them, and posts a diff when you're done, and if the author accepts it out becomes a jointly authored commit.

In situations where I'm doing PR reviews seriously in a multi-developer environment I'm usually on Gitlab. Gitlab definitely has some features for this sort of thing, but I've never been able to use them seamlessly or quickly.

I was at one point considering writing a tool which would checkout an MR, then let you just edit it as per normal, and then would submit the whole thing back to a Gitlab MR as a set of proposed changes. The point was to ensure that you could easily expand the MR beyond the diff of changed lines, which was frequently inadequate to review a patch properly since it omitted context.

One way to do that is to open a second PR which targets the branch of the first PR. I think that's also probably the most natural way -- would you agree? I don't see it done much, but I've done it once or twice and I'd embrace it if it were more culturally standard.

That's what the parent comment is talking about though: and I agree it's better, but it just pops as another global PR.

The UI doesn't know what to do with it, and most developers don't really understand it without spending a bunch of time explaining it.

When it works, it works great but the overhead is ridiculous since you get nothing inline on the UI: ideally something like the commits should pop up as comments, with accept/reject/discuss options.

Right, agree with all that.

I frequently use “suggestion” to provide a change I think would be better.

‘’’suggestion <code block> ‘’’

Reply on news.ycombinator.com