[@types/underscore] Collection and Array Tests - Find and Reject by reubenrybnik · Pull Request #45994 · DefinitelyTyped/DefinitelyTyped

reubenrybnik · 2020-07-10T07:35:10Z

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 find and reject.
Updates find and reject to use Iteratee.
Removes the redeclaration of the T generic in Underscore.find and _Chain.find.
Updates the return type of _Chain.reject 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 detect overloads with a reference to find overloads.
Updates TypeOfCollection to extract any instead of unknown as the collection item type for any for better backwards compatibility.
Updates filter to support an identity iteratee.
Fixes a bit of summary comment indentation that I missed in previous PRs and updates the @returns for filter to better match the one for reject.

…ssed previously.

typescript-bot · 2020-07-10T07:35:14Z

@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": 45994,
  "author": "reubenrybnik",
  "owners": [
    "borisyankov",
    "jbaldwin",
    "ccurrens",
    "confususs",
    "jgonggrijp",
    "ffflorian",
    "regevbr",
    "peterblazejewicz",
    "reubenrybnik"
  ],
  "dangerLevel": "ScopedAndTested",
  "headCommitAbbrOid": "71bbdd7",
  "headCommitOid": "71bbdd7fc4e25219a16c9985198471882e738c8c",
  "mergeIsRequested": true,
  "stalenessInDays": 0,
  "lastPushDate": "2020-07-10T20:48:23.000Z",
  "lastCommentDate": "2020-07-11T00:00:29.000Z",
  "maintainerBlessed": false,
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/45994/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-10T21:27:28.000Z",
  "reviewersWithStaleReviews": [],
  "approvalFlags": 2,
  "isChangesRequested": false
}

typescript-bot · 2020-07-10T07:35:15Z

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

…roofing because even though this works as-is it seems like any should pass an extends List or extends Dictionary check.

types/underscore/index.d.ts

jgonggrijp

This is starting to feel smooth. Nice work. "Request changes" in this case is mostly just a request for additional tests, the rest is pretty minor.

types/underscore/index.d.ts

types/underscore/underscore-tests.ts

types/underscore/index.d.ts

typescript-bot · 2020-07-10T09:22:36Z

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

typescript-bot · 2020-07-10T09:50:56Z

👋 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	#45994	diff
Batch compilation
Memory usage (MiB)	60.2	54.8	-9.0%
Type count	10091	9613	-5%
Assignability cache size	1612	1490	-8%

Language service
Samples taken	4333	4135	-5%
Identifiers in tests	4333	4135	-5%
`getCompletionsAtPosition`
Mean duration (ms)	177.5	177.1	-0.3%
Mean CV	10.1%	10.5%
Worst duration (ms)	332.1	337.5	+1.6%
Worst identifier	_	_
`getQuickInfoAtPosition`
Mean duration (ms)	169.8	169.9	+0.1%
Mean CV	10.3%	10.6%	+3.3%
Worst duration (ms)	327.9	340.4	+3.8%
Worst identifier	extractUnderscoreTypes	extractChainTypes

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

…reject and alternating between lists and dictionaries for non-function iteratees since covering both cases for those iteratees is not as interesting.

typescript-bot · 2020-07-10T18:54:39Z

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

…erred and adding any tests for find, filter, and reject.

jgonggrijp

I'm content, just a few more nitpicks. Especially two tests where I suspect you forgot to remove the stringRecordProperty arguments.

types/underscore/underscore-tests.ts

…entity iteratee.

Co-authored-by: Julian Gonggrijp <dev@juliangonggrijp.com>

reubenrybnik · 2020-07-10T21:15:12Z

From my company's solution, I get two new errors. For the first one, we're using find like each but returning nothing rather than a boolean that would act like a break statement (which it looks like worked in the previous definition by inappropriately falling back to the iterator: (U extends {}) overload). For the second, we're explicitly specifying a generic when we don't need to, which breaks because the generic changed from T to V. Both seem like reasonable errors to me.

jgonggrijp

From my company's solution, I get two new errors. For the first one, we're using find like each but returning nothing rather than a boolean that would act like a break statement (which it looks like worked in the previous definition by inappropriately falling back to the iterator: (U extends {}) overload).

Out of curiosity, is there a particular reason to use find as each?

For the second, we're explicitly specifying a generic when we don't need to, which breaks because the generic changed from T to V. Both seem like reasonable errors to me.

I agree. Again, good job and thanks for your hard work!

reubenrybnik · 2020-07-10T21:33:47Z

Thanks @jgonggrijp!

Underscore documentation mentions using find as a way to be able to break out of a loop early (see the note part of the image below).

jgonggrijp · 2020-07-10T23:36:03Z

Yes I know that, in fact find is the functional programming counterpart of a for loop with break. But you explicitly mentioned that you were using find instead of each with an iteratee that returns nothing, meaning that you are not, in fact, using find to implement partial iteration. In that case, there is no difference with each, so I wondered why you were still using find.

reubenrybnik · 2020-07-11T00:00:09Z

We should ideally be returning a Boolean but the person that wrote it missed doing that, so it's a good error that I'm glad is now happening with these updates and that I look forward to fixing 😄

Happily it's just slightly more inefficient than it could be rather than being wrong in a dangerous way.

reubenrybnik · 2020-07-11T00:00:29Z

Ready to merge

typescript-bot · 2020-07-11T00:26:56Z

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

@reubenrybnik

…rray Tests - Find and Reject by @reubenrybnik * Updating type definitions for find and reject and adding tests. * Updating summary comment indentation for a few see comments that I missed previously. * Updating TypeOfCollection<any> to result in any instead of unknown. * Moving the never check up to the top of TypeOfCollection for future-proofing because even though this works as-is it seems like any should pass an extends List or extends Dictionary check. * Adding support and tests for identity iteratees in find, filter, and reject and alternating between lists and dictionaries for non-function iteratees since covering both cases for those iteratees is not as interesting. * Adding any and never tests to OOP and chain. * Summary comment updates suggested by @jgonggrijp. * Adding a boolean iterator that won't cause collection types to be inferred and adding any tests for find, filter, and reject. * Fixing identity iteratee tests that I didn't properly switch to an identity iteratee. * Remove a filter designation from a reject test group. Co-authored-by: Julian Gonggrijp <dev@juliangonggrijp.com> Co-authored-by: Julian Gonggrijp <dev@juliangonggrijp.com>

@reubenrybnik

…rray Tests - Find and Reject by @reubenrybnik * Updating type definitions for find and reject and adding tests. * Updating summary comment indentation for a few see comments that I missed previously. * Updating TypeOfCollection<any> to result in any instead of unknown. * Moving the never check up to the top of TypeOfCollection for future-proofing because even though this works as-is it seems like any should pass an extends List or extends Dictionary check. * Adding support and tests for identity iteratees in find, filter, and reject and alternating between lists and dictionaries for non-function iteratees since covering both cases for those iteratees is not as interesting. * Adding any and never tests to OOP and chain. * Summary comment updates suggested by @jgonggrijp. * Adding a boolean iterator that won't cause collection types to be inferred and adding any tests for find, filter, and reject. * Fixing identity iteratee tests that I didn't properly switch to an identity iteratee. * Remove a filter designation from a reject test group. Co-authored-by: Julian Gonggrijp <dev@juliangonggrijp.com> Co-authored-by: Julian Gonggrijp <dev@juliangonggrijp.com>

reubenrybnik added 3 commits July 9, 2020 23:14

Updating type definitions for find and reject and adding tests.

c20b340

Updating summary comment indentation for a few see comments that I mi…

761d36d

…ssed previously.

Updating TypeOfCollection<any> to result in any instead of unknown.

cec8ac7

reubenrybnik requested a review from borisyankov as a code owner July 10, 2020 07:35

typescript-bot added Where is GH Actions? GH Actions didn't give a response to this PR Popular package This PR affects a popular package (as counted by NPM download counts). Author is Owner The author of this PR is a listed owner of the package. labels Jul 10, 2020

typescript-bot removed the Where is GH Actions? GH Actions didn't give a response to this PR label Jul 10, 2020

Moving the never check up to the top of TypeOfCollection for future-p…

9f5f0b8

…roofing because even though this works as-is it seems like any should pass an extends List or extends Dictionary check.

reubenrybnik commented Jul 10, 2020

View reviewed changes

types/underscore/index.d.ts Show resolved Hide resolved

jgonggrijp suggested changes Jul 10, 2020

View reviewed changes

typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Jul 10, 2020

typescript-bot added Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. and removed Revision needed This PR needs code changes before it can be merged. labels Jul 10, 2020

reubenrybnik added 3 commits July 10, 2020 10:36

Adding support and tests for identity iteratees in find, filter, and …

154352f

…reject and alternating between lists and dictionaries for non-function iteratees since covering both cases for those iteratees is not as interesting.

Adding any and never tests to OOP and chain.

da607ef

Summary comment updates suggested by @jgonggrijp.

b4f42b5

Adding a boolean iterator that won't cause collection types to be inf…

6cfb95e

…erred and adding any tests for find, filter, and reject.

jgonggrijp suggested changes Jul 10, 2020

View reviewed changes

types/underscore/underscore-tests.ts Outdated Show resolved Hide resolved

types/underscore/underscore-tests.ts Outdated Show resolved Hide resolved

types/underscore/underscore-tests.ts Outdated Show resolved Hide resolved

types/underscore/underscore-tests.ts Show resolved Hide resolved

reubenrybnik and others added 2 commits July 10, 2020 13:47

Fixing identity iteratee tests that I didn't properly switch to an id…

1a3e5af

…entity iteratee.

Remove a filter designation from a reject test group.

71bbdd7

Co-authored-by: Julian Gonggrijp <dev@juliangonggrijp.com>

jgonggrijp approved these changes Jul 10, 2020

View reviewed changes

typescript-bot added Owner Approved A listed owner of this package signed off on the pull request. Self Merge This PR can now be self-merged by the PR author or an owner labels Jul 10, 2020

typescript-bot merged commit bbe969c into DefinitelyTyped:master Jul 11, 2020

reubenrybnik mentioned this pull request Jul 19, 2020

[@types/underscore] More comprehensive tests for Underscore collection and array functions + definition changes as appropriate #45201

Closed

10 tasks

Conversation

reubenrybnik commented Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Reviews

Status

Uh oh!

typescript-bot commented Jul 10, 2020

Uh oh!

Uh oh!

jgonggrijp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

typescript-bot commented Jul 10, 2020

Uh oh!

typescript-bot commented Jul 10, 2020

Uh oh!

typescript-bot commented Jul 10, 2020

Uh oh!

jgonggrijp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

reubenrybnik commented Jul 10, 2020

Uh oh!

jgonggrijp left a comment

Choose a reason for hiding this comment

Uh oh!

reubenrybnik commented Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jgonggrijp commented Jul 10, 2020

Uh oh!

reubenrybnik commented Jul 11, 2020

Uh oh!

reubenrybnik commented Jul 11, 2020

Uh oh!

typescript-bot commented Jul 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

reubenrybnik commented Jul 10, 2020 •

edited

Loading

typescript-bot commented Jul 10, 2020 •

edited

Loading

reubenrybnik commented Jul 10, 2020 •

edited

Loading