@reubenrybnik Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

• underscore on npm, on unpkg

Code Reviews

Because you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it.

Status

• ✅ No merge conflicts
• ✅ Continuous integration tests have passed
• ✅ Most recent commit is approved by type definition owners or DT maintainers

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.

{
  "type": "info",
  "now": "-",
  "pr_number": 47543,
  "author": "reubenrybnik",
  "owners": [
    "borisyankov",
    "jbaldwin",
    "ccurrens",
    "confususs",
    "jgonggrijp",
    "ffflorian",
    "regevbr",
    "peterblazejewicz",
    "reubenrybnik"
  ],
  "dangerLevel": "ScopedAndTested",
  "headCommitAbbrOid": "7cbc73a",
  "headCommitOid": "7cbc73a9a5ea6ce47b53e4a7c268a615cd62851a",
  "mergeIsRequested": true,
  "stalenessInDays": 0,
  "lastPushDate": "2020-09-25T04:36:25.000Z",
  "reopenedDate": "2020-09-25T04:36:45.000Z",
  "lastCommentDate": "2020-09-25T16:57:23.000Z",
  "maintainerBlessed": false,
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/47543/files",
  "hasMergeConflict": false,
  "authorIsOwner": true,
  "isFirstContribution": false,
  "popularityLevel": "Popular",
  "newPackages": [],
  "packages": [
    "underscore"
  ],
  "files": [
    {
      "path": "types/underscore/index.d.ts",
      "kind": "definition",
      "package": "underscore"
    },
    {
      "path": "types/underscore/underscore-tests.ts",
      "kind": "test",
      "package": "underscore"
    }
  ],
  "hasDismissedReview": false,
  "ciResult": "pass",
  "lastReviewDate": "2020-09-25T09:50:06.000Z",
  "reviewersWithStaleReviews": [],
  "approvalFlags": 2,
  "isChangesRequested": false
}

🔔 @borisyankov @jbaldwin @ccurrens @confususs @jgonggrijp @ffflorian @regevbr @peterblazejewicz — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

These ended up being way more naturally organized than I thought they were (I hadn't originally anticipated being able to divide these into sections - at least not without moving a lot more stuff around), and except for chaining, each, reduce*, and map there was generally only one test function per Underscore function. In retrospect, I wish I would have cleaned up the existing tests for each function as I was adding their combinatorial tests rather than doing a larger cleanup PR like this.

I could potentially move the usage examples for each function to just above their corresponding combinatorial examples if folks think that that would be a better way to organize this file.

👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings.

Let’s review the numbers, shall we?

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

@reubenrybnik One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. If you disagree with the reviewer's comments, you can "dismiss" the review using GitHub's review UI. Thank you!

Works for me @jgonggrijp! I think I'll go C then A then B, but I think I'll need to move the removal of EnumerableKey into C. I'll put this into a draft status, force push it back to its original base, use it as an aggregate branch to merge the sub-PRs into, and reopen this for final merge once everything's back in it.

The original commits and diff will be available for reference in master...reubenrybnik:underscore-tests-usage-tests-cleanup-old.

@reubenrybnik — There was an error that prevented me from properly processing this PR:

No head commit found

Well that didn't work, won't try to do that again 😞

To be fair, I didn't force push it back to its original base like I said, instead I forced it to current master. My plan might have worked if I had stuck with the original base and then merged current master in instead.

Looks like this is in a special place on the PR board with a special label now; to the DT maintainer that will likely end up looking at this, please feel free to drop this PR off the board, and sorry for getting this into an unpleasant state that you've needed to come and investigate.

Huzzah, I did what I said was what I probably should have done and fixed it and now DT maintainers won't need to come visit! 😬

Edit: Except this still has the error label, so maybe I'll get a visit anyway. 😬 😬 😬 At least I know the right and wrong way to do this now.

On the bright side, maybe they have enough PRs with an error label that they don't inspect each of them individually. ;-)

@jgonggrijp The first child PR is up at reubenrybnik#1 😄

I've invited you to be a collaborator in my fork, which was hopefully the right thing to do; let me know if there's something different that I should be doing to provide you with the power to effectively review PRs in my fork.

@reubenrybnik Since your fork is public, I probably didn’t need any type of special permission in order to review your PRs. In public repos, everyone can review everything. I have accepted the invitation, but I might never use it. I take no offense if you remove my access again.

@jgonggrijp Meh, might as well leave things as they are for the moment at least to be safe, this way too I can directly request you as a reviewer which is nice. I'll only do that for PRs like these that you know about in advance, though.

@jgonggrijp Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

Yay, the PR is fully back in a good state! Excellent job dealing with my fail @typescript-bot 😅

@reubenrybnik Everything looks good here. Great job! I am ready to merge this PR on your behalf.

If you'd like that to happen, please post a comment saying:

Ready to merge

and I'll merge this PR almost instantly. Thanks for helping out! ❤️

(@borisyankov, @jbaldwin, @ccurrens, @confususs, @jgonggrijp, @ffflorian, @regevbr, @peterblazejewicz: you can do this too.)

Thanks @jgonggrijp! I appreciate your responsiveness during this round of reviews, your willingness to review so many PRs, and your feedback and ideas for improvement which provided a substantial quality boost to the result of this effort.

I look forward to working with you again as well, quite possibly on more improvements to these type definitions since there are definitely more that can be made both now and as future versions of TS become available but hopefully in other areas of the open-source world too 😄

Ready to merge

I just published @types/underscore@1.10.24 to npm.