←back to thread

446 points talboren | 2 comments | | HN request time: 0.001s | source
Show context
muglug ◴[] No.45039093[source]
Improvements merged within the last two days by the WebKit team: https://github.com/orgs/community/discussions/170922#discuss...

For my sins I occasionally create large PRs (> 1,000 files) in GitHub, and teammates (who mostly all use Chrome) will sometimes say "I'll approve once it loads for me..."

replies(6): >>45039371 #>>45039546 #>>45039877 #>>45040882 #>>45043276 #>>45050622 #
patrickmay ◴[] No.45039371[source]
That seems essentially unreviewable. If you can share without violating an NDA, what kind of PR would involve that many files?
replies(6): >>45039414 #>>45039433 #>>45039455 #>>45039478 #>>45040667 #>>45041353 #
bob1029 ◴[] No.45039478[source]
"Upgrade solution from .NET Framework 4.8 => .NET 8"

"Rename 'CustomerEmailAddress' to 'CustomerEmail'"

"Upgrade 3rd party API from v3 to v4"

I genuinely don't get this notion of a "max # of files in a PR". It all comes off to me as post hoc justification of really shitty technology decisions at GitHub.

replies(2): >>45039963 #>>45041641 #
ambicapter ◴[] No.45039963[source]
It's not GitHub-specific advice, it's about reviewability of the PR vs. human working memory/maximum attention span.
replies(2): >>45040291 #>>45041083 #
eviks ◴[] No.45040291[source]
How much working memory/attention span is required to look through 1000 identical lines "-CustomerEmailAddress +CustomerEmail"?
replies(1): >>45040910 #
shadowgovt ◴[] No.45040910[source]
Ideally, you automate a check like that. Because the answer turns out to actually be "humans are profoundly bad at that kind of pattern recognition."

A computer will be able to tell that the 497th has a misspelled `CusomerEmail` or that change 829 is a regexp failure that trimmed the boolean "CustomerEmailAddressed" to "CustomerEmailed" with 100% reliability; humans, not so much.

replies(3): >>45041140 #>>45041171 #>>45047557 #
eviks ◴[] No.45041171[source]
Oh, certainly, didn't mean that you had to avoid using your IDE to autorename a variable yourself (to avoid the boolean issue) and diffed results to those of the PR

Or that you had to avoid Ctrl+F "CustomerEmail" and see whether you had 1000 matches that matches the number of changed files or only 999 due to some typo.

Or using the web interface to filter by file type to batch your reviews.

Or...

Just that in none of those cases there is anything close to our memory/attention capacity.

replies(2): >>45041585 #>>45042397 #
1. shadowgovt ◴[] No.45042397[source]
I envy your IDE being able to do a rename of that scale.

I work in a large C++ codebase and a rename like that will actually just crash my vscode instance straight-up.

(There are good automated tools that make it straightforward to script up a repository-wide mutation like this however. But they still generate PRs that require human review; in the case of the one I used, it'd break the PR up into tranches of 50-ish files per tranche and then hunt down individuals with authority to review the root directory of the tranche and assign it to them. Quite useful!)

replies(1): >>45043353 #
2. trenchpilgrim ◴[] No.45043353[source]
Yeah VSCode is pretty terrible at refactorings that Jetbrains or Zed will do basically instantly.