5 ms·

That seems essentially unreviewable. If you can share without violating an NDA, what kind of PR would involve that many files?

trenchpilgrim 1y ago
Ones where you have a lot of generated files you commit into Git, and you change the output of the generator tool.
codezero 1y ago
Can’t speak for the person above but we keep a lot of configuration files in git and could easily write a thousand new configs in a single PR, or adding a new key to all the configs for example.
scsh 1y ago
If the project you're working on vendors dependencies it's pretty easy to end up with that many files being changed when adding or updating, even when trying to make as narrow updates as possible in one PR.
bob1029 1y ago
"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.
- ambicapter 1y ago
  It's not GitHub-specific advice, it's about reviewability of the PR vs. human working memory/maximum attention span.
  - eviks 1y ago
    How much working memory/attention span is required to look through 1000 identical lines "-CustomerEmailAddress +CustomerEmail"?
    - shadowgovt 1y ago
      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.
      - makeitdouble 1y ago
        You're not just reviewing the individual lines, but also which context, and which files are impacted. And automating that part would still mean reviewing the automation and the 1000+ changes to move to it. Sure 1000+ changes kills the soul, we're not good at that, but sometimes there's just no other decent choice.
      - eviks 1y ago
        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.
        deleted 1y ago
        [deleted]
        shadowgovt 1y ago
        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!)
  - sidewndr46 1y ago
    I pretty frequently have conversations with other engineers where I point out that a piece of code makes an assumption that mostly holds true, but doesn't always hold true. Hence, a user visible bug. The usual response is something like "if you're correct, wouldn't that mean there are hundreds of cases where this needs to be fixed to resolve this bug?". The answer obviously being yes. Incoming 100+ file PR to resolve this issue. I have no other ideas for how someone is supposed to resolve an issue in this scenario
- bityard 1y ago
  I've always thought those kinds of large-scale search-and-replace diffs should not generally be expected to be reviewed. If a review is 1000's of lines of identical changes (or newly-vendored code), literally nobody is actually reading it, even if they are somehow able to convince themselves that they are. I would rather just see the steps you ran to generate the diff and review that instead.
cesarb 1y ago
> what kind of PR would involve that many files? A very simple example: migrating from JavaEE to JakartaEE. Every single Java source file has to have the imports changed from "javax." to "jakarta.", which can easily be thousands of files. It's also easy to review (and any file which missed that change will fail when compiling on the CI).
moffkalast 1y ago
Convert space indents to tabs, as god intended.
- itsgabriel 1y ago
  as god indented, you mean?
  - moffkalast 1y ago
    I'm stealing that one, lmao

trenchpilgrim 1y ago

Ones where you have a lot of generated files you commit into Git, and you change the output of the generator tool.

codezero 1y ago

Can’t speak for the person above but we keep a lot of configuration files in git and could easily write a thousand new configs in a single PR, or adding a new key to all the configs for example.

scsh 1y ago

If the project you're working on vendors dependencies it's pretty easy to end up with that many files being changed when adding or updating, even when trying to make as narrow updates as possible in one PR.

bob1029 1y ago

"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.

ambicapter 1y ago
It's not GitHub-specific advice, it's about reviewability of the PR vs. human working memory/maximum attention span.
- eviks 1y ago
  How much working memory/attention span is required to look through 1000 identical lines "-CustomerEmailAddress +CustomerEmail"?
  - shadowgovt 1y ago
    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.
    - makeitdouble 1y ago
      You're not just reviewing the individual lines, but also which context, and which files are impacted. And automating that part would still mean reviewing the automation and the 1000+ changes to move to it. Sure 1000+ changes kills the soul, we're not good at that, but sometimes there's just no other decent choice.
    - eviks 1y ago
      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.
      - deleted 1y ago
        [deleted]
      - shadowgovt 1y ago
        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!)
- sidewndr46 1y ago
  I pretty frequently have conversations with other engineers where I point out that a piece of code makes an assumption that mostly holds true, but doesn't always hold true. Hence, a user visible bug. The usual response is something like "if you're correct, wouldn't that mean there are hundreds of cases where this needs to be fixed to resolve this bug?". The answer obviously being yes. Incoming 100+ file PR to resolve this issue. I have no other ideas for how someone is supposed to resolve an issue in this scenario
bityard 1y ago
I've always thought those kinds of large-scale search-and-replace diffs should not generally be expected to be reviewed. If a review is 1000's of lines of identical changes (or newly-vendored code), literally nobody is actually reading it, even if they are somehow able to convince themselves that they are. I would rather just see the steps you ran to generate the diff and review that instead.

cesarb 1y ago

> what kind of PR would involve that many files? A very simple example: migrating from JavaEE to JakartaEE. Every single Java source file has to have the imports changed from "javax." to "jakarta.", which can easily be thousands of files. It's also easy to review (and any file which missed that change will fail when compiling on the CI).

moffkalast 1y ago

Convert space indents to tabs, as god intended.

itsgabriel 1y ago
as god indented, you mean?
- moffkalast 1y ago
  I'm stealing that one, lmao