[@types/underscore] Collection and Array Tests - Each, ToArray, and Partition

[reubenrybnik]

• Use a meaningful title for the pull request. Include the name of the package modified.
• Test the change in your own code. (Compile and run.)
• Add or edit tests to reflect the change. (Run with npm test.)
• Follow the advice from the readme.
• Avoid common mistakes.
• Run npm run lint package-name (or tsc if no tslint.json is present).
• Provide a URL to documentation or source code which provides context for the suggested changes: https://underscorejs.org/
• Include tests for your changes

This PR continues the planned set that will together add up to #45201 and includes the following changes:

• Adds tests for each, toArray, and partition.
• Collapses overloads of each by updating it to use CollectionIterator.
• Updates the return value of each to be the exact original collection type rather than an approximation.
• Updates partition to use Collection and Iteratee to partially fix Underscore collections functions should take objects #20623.
• Updates partition to result in a two-item tuple instead of an array.
• Updates the location of Underscore.partition and _Chain.partition to be in the Collections sections of those interfaces.
• Updates the return type of _Chain.each, _Chain.toArray, and _Chain.partition to use the correct wrapped value type V to partially fix @types/underscore error TS2322 for _.chain after upgrade to v1.9 #36308.
• Updates the Underscore and _Chain versions of reject to work with both Lists and Dictionaries to partially fix Underscore collections functions should take objects #20623.
• Replaces all forEach overloads with a reference to each overloads.
• Fixes issues with CollectionIterator and MemoCollectionIterator around element type inference with collections that are type unions or any.

[typescript-bot]

@reubenrybnik Thank you for submitting this PR!

This is a live comment which I will keep updated.

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.

---

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 46120,
  "author": "reubenrybnik",
  "owners": [
    "borisyankov",
    "jbaldwin",
    "ccurrens",
    "confususs",
    "jgonggrijp",
    "ffflorian",
    "regevbr",
    "peterblazejewicz",
    "reubenrybnik"
  ],
  "dangerLevel": "ScopedAndTested",
  "headCommitAbbrOid": "5ef8fe2",
  "headCommitOid": "5ef8fe2907257beac41e27c3dc2399a087eddb67",
  "mergeIsRequested": true,
  "stalenessInDays": 0,
  "lastPushDate": "2020-07-17T14:12:06.000Z",
  "lastCommentDate": "2020-07-19T20:17:06.000Z",
  "maintainerBlessed": false,
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/46120/files",
  "hasMergeConflict": false,
  "authorIsOwner": true,
  "isFirstContribution": false,
  "popularityLevel": "Popular",
  "anyPackageIsNew": false,
  "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-07-19T20:19:03.000Z",
  "reviewersWithStaleReviews": [],
  "approvalFlags": 2,
  "isChangesRequested": false
}

[typescript-bot]

🔔 @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.

[typescript-bot]

👋 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?

Comparison details 📊

	master	#46120	diff
Batch compilation
Memory usage (MiB)	64.1	62.6	-2.4%
Type count	10127	9725	-4%
Assignability cache size	1540	1145	-26%
Language service
Samples taken	4836	5205	+8%
Identifiers in tests	4836	5205	+8%
getCompletionsAtPosition
Mean duration (ms)	147.9	146.6	-0.9%
Mean CV	11.6%	11.4%
Worst duration (ms)	273.3	255.5	-6.5%
Worst identifier	context	_
getQuickInfoAtPosition
Mean duration (ms)	142.3	141.0	-1.0%
Mean CV	12.0%	11.5%	-3.9%
Worst duration (ms)	286.9	269.7	-6.0%
Worst identifier	extractChainTypes	extractUnderscoreTypes

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

[jgonggrijp]

I mostly agree with the changes, just a few details.

@types/underscore has improved a lot since you started this project and it is a joy to see it still moving forward. Keep it up. 👍

[typescript-bot]

@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!

[jgonggrijp]

I think we're done here. I look forward to the next one!

[reubenrybnik]

Thanks @jgonggrijp! I appreciate your attentiveness to these reviews and the ideas you've provided throughout this process that have helped to make these changes much better than I'd originally planned. From a PR count perspective, this puts us at about the halfway point in my current planned set (which has changed a little from the set I originally mentioned in #45304).

I'll wait until at least 7PM UTC tomorrow 7/19 to merge this just in case you'd like to make any further replies to my spew about Falsy, and I'll try to do a better job of organizing my thoughts and double-checking my work more thoroughly before replying in the future since I feel like there have been a couple of conversations so far during this process where I've ended up going off the rails a bit.

[jgonggrijp]

Thanks @jgonggrijp! I appreciate your attentiveness to these reviews and the ideas you've provided throughout this process that have helped to make these changes much better than I'd originally planned.

Glad to hear that! That's the main reason for doing it so I'm glad it works.

From a PR count perspective, this puts us at about the halfway point in my current planned set (which has changed a little from the set I originally mentioned in #45304).

Only halfway, haha.

I'll wait until at least 7PM UTC tomorrow 7/19 to merge this just in case you'd like to make any further replies to my spew about Falsy,

I made a small reply. Unless you want to make further decisions depend on whatever discussion might come after that, the rest of the "grace period" is not necessary as far as I'm concerned.

and I'll try to do a better job of organizing my thoughts and double-checking my work more thoroughly before replying in the future since I feel like there have been a couple of conversations so far during this process where I've ended up going off the rails a bit.

It's always worth striving to better organize your thoughts and to double-check your work, but please don't be so hard on yourself. I'm quite sure I've made a few mistakes as well, especially when it comes to realism about what the TS compiler will do. Humans in general are (intensely) fallible, that's why we tend to get so much better results when we team up.

By the way, I've been tweeting about our reviews and the tweets seem to attract some interest. If you happen to be on Twitter, it may be nice to associate. (I tried finding you but couldn't be sure.)

[reubenrybnik]

Only halfway, haha.

We are probably more than halfway along with regards to effort though 🎉

I made a small reply. Unless you want to make further decisions depend on whatever discussion might come after that, the rest of the "grace period" is not necessary as far as I'm concerned.

Sounds good, I'll hit the go button now then 😄

By the way, I've been tweeting about our reviews and the tweets seem to attract some interest. If you happen to be on Twitter, it may be nice to associate. (I tried finding you but couldn't be sure.)

Sorry to disappoint here, but while I made a Twitter account once upon a time and so can technically be said to exist there I've never actually tweeted with it (and off the top of my head I don't even remember my handle). Happy to hear, though, that others might be finding this work to be valuable or interesting 😁

[reubenrybnik]

Ready to merge

[reubenrybnik]

Hmm, sorry @jgonggrijp, I didn't add any commits but somehow something one of us did surprisingly seems to have caused your approval to not apply anymore as indicated by this tag removal. If you wouldn't mind clicking approve one more time on this, I'd appreciate it!

[jgonggrijp]

Curious. Well, here we go again!

[typescript-bot]

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

[reubenrybnik]

Hmm, another interesting interaction here, not the biggest fan of how my previous "Ready to merge" caused this to be merged at the moment of your re-approval since I feel like a merge should only be triggered when the author is the one taking an action 😕

[jgonggrijp]

That was probably meant to be smart. But I agree it's unexpected.