[@types/underscore] Collection and Array Tests - Zip, Unzip, and Object by reubenrybnik · Pull Request #46306 · DefinitelyTyped/DefinitelyTyped

reubenrybnik · 2020-07-24T02:16:15Z

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 zip, unzip, and object.
Removes an overload of UnderscoreStatic.zip that doesn't seem like it's correct.
Updates unzip functions to not take more than a single input (either a single parameter or the wrapped value).
Collapses object to a single overload that always results in a Dictionary of some sort rather than allowing the caller to specify a more exact result through generics and infers its resulting value type when it can, falling back to any when it can't.
Updates the return type of the _Chain functions to use the correct wrapped value type V (these are more drastic changes than those that partially fix @types/underscore error TS2322 for _.chain after upgrade to v1.9 #36308 though).

typescript-bot · 2020-07-24T02:16:28Z

@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": 46306,
  "author": "reubenrybnik",
  "owners": [
    "borisyankov",
    "jbaldwin",
    "ccurrens",
    "confususs",
    "jgonggrijp",
    "ffflorian",
    "regevbr",
    "peterblazejewicz",
    "reubenrybnik"
  ],
  "dangerLevel": "ScopedAndTested",
  "headCommitAbbrOid": "db3c2d5",
  "headCommitOid": "db3c2d5aebf4f55de033a060b7f1b1cdec9ad1f1",
  "mergeIsRequested": true,
  "stalenessInDays": 0,
  "lastPushDate": "2020-07-27T23:22:51.000Z",
  "lastCommentDate": "2020-07-28T13:10:18.000Z",
  "maintainerBlessed": false,
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/46306/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-28T07:23:29.000Z",
  "reviewersWithStaleReviews": [],
  "approvalFlags": 2,
  "isChangesRequested": false
}

typescript-bot · 2020-07-24T02:16:29Z

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

types/underscore/index.d.ts

typescript-bot · 2020-07-24T04:20:40Z

👋 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	#46306	diff
Batch compilation
Memory usage (MiB)	55.8	57.0	+2.2%
Type count	11574	11734	+1%
Assignability cache size	1407	1431	+2%

Language service
Samples taken	6467	6553	+1%
Identifiers in tests	6467	6553	+1%
`getCompletionsAtPosition`
Mean duration (ms)	192.3	184.3	-4.2%
Mean CV	8.6%	8.7%
Worst duration (ms)	373.6	353.5	-5.4%
Worst identifier	filter	_
`getQuickInfoAtPosition`
Mean duration (ms)	187.5	179.8	-4.1%
Mean CV	8.8%	9.0%	+2.2%
Worst duration (ms)	382.9	356.4	-6.9%
Worst identifier	extractChainTypes	extractChainTypes

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

jgonggrijp

Great job again. I would prefer separate overloads for the one- and two-argument signatures of object, but otherwise I'm very happy with these changes.

types/underscore/index.d.ts

types/underscore/underscore-tests.ts

typescript-bot · 2020-07-26T13:28:40Z

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

…y/value lists and making the result Dictionary<never> when a list of incorrect depth is supplied.

typescript-bot · 2020-07-26T17:44:47Z

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

…y that it's private since unlike most of the types in this file this is really a type that's specifically a helper for the object function which typically shouldn't be used anywhere else.

…needed anymore.

types/underscore/index.d.ts

types/underscore/underscore-tests.ts

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

typescript-bot · 2020-07-27T07:54:27Z

@reubenrybnik The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

reubenrybnik · 2020-07-27T09:23:57Z

@jgonggrijp I've made all the revisions I plan to for now here if you wouldn't mind taking another look whenever you have a moment. As usual, thanks for all the valuable feedback!

jgonggrijp

This is better again, but I have one concern. See below.

If you disagree and you manage to convince me, I'll change the verdict to "approve".

types/underscore/index.d.ts

typescript-bot · 2020-07-27T22:21:15Z

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

reubenrybnik · 2020-07-27T23:09:37Z

Ignore this, I'm just trying out adding a comment after marking all items resolved to see if anything happens.

reubenrybnik · 2020-07-27T23:13:28Z

Nope.

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

jgonggrijp

You convinced me about the constraint, but given that TS is not having it anyway, I also agree that it is still better to have it in place.

Again, good job!

typescript-bot · 2020-07-28T07:24:34Z

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

reubenrybnik · 2020-07-28T13:10:01Z

No new errors occur in my company's solution with this final set of changes.

reubenrybnik · 2020-07-28T13:10:18Z

Ready to merge

typescript-bot · 2020-07-28T13:47:17Z

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

@reubenrybnik

…rray Tests - Zip, Unzip, and Object by @reubenrybnik * Updating type definitions for zip, unzip, and object and adding tests. * Adding undefined to possible results of object when given separate key/value lists and making the result Dictionary<never> when a list of incorrect depth is supplied. * Adding tests for single arguments for zip. * Adding comments that help to explain the logic of PairValue. * Splitting PairValue into two separate types instead of nesting logic. * Putting an underscore in front of the ObjectValue type to help signify that it's private since unlike most of the types in this file this is really a type that's specifically a helper for the object function which typically shouldn't be used anywhere else. * Removing a portion of the summary comments for zip since they aren't needed anymore. * Infer the pair list type when possible instead of using `any` Co-authored-by: Julian Gonggrijp <dev@juliangonggrijp.com> * Making object into separate overloads. * Constraining the list of keys to be members of EnumerableKey. Co-authored-by: Julian Gonggrijp <dev@juliangonggrijp.com> Co-authored-by: Julian Gonggrijp <dev@juliangonggrijp.com>

@reubenrybnik

…rray Tests - Zip, Unzip, and Object by @reubenrybnik * Updating type definitions for zip, unzip, and object and adding tests. * Adding undefined to possible results of object when given separate key/value lists and making the result Dictionary<never> when a list of incorrect depth is supplied. * Adding tests for single arguments for zip. * Adding comments that help to explain the logic of PairValue. * Splitting PairValue into two separate types instead of nesting logic. * Putting an underscore in front of the ObjectValue type to help signify that it's private since unlike most of the types in this file this is really a type that's specifically a helper for the object function which typically shouldn't be used anywhere else. * Removing a portion of the summary comments for zip since they aren't needed anymore. * Infer the pair list type when possible instead of using `any` Co-authored-by: Julian Gonggrijp <dev@juliangonggrijp.com> * Making object into separate overloads. * Constraining the list of keys to be members of EnumerableKey. Co-authored-by: Julian Gonggrijp <dev@juliangonggrijp.com> Co-authored-by: Julian Gonggrijp <dev@juliangonggrijp.com>

Updating type definitions for zip, unzip, and object and adding tests.

ef3af0e

reubenrybnik requested a review from borisyankov as a code owner July 24, 2020 02:16

typescript-bot added 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 24, 2020

reubenrybnik commented Jul 24, 2020

View reviewed changes

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

reubenrybnik commented Jul 24, 2020

View reviewed changes

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

reubenrybnik commented Jul 24, 2020

View reviewed changes

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

typescript-bot added the Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. label Jul 24, 2020

jgonggrijp mentioned this pull request Jul 26, 2020

Transposing matrices using zip/unzip jashkenas/underscore#2867

Closed

jgonggrijp suggested changes Jul 26, 2020

View reviewed changes

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

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

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

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

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

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

Adding undefined to possible results of object when given separate ke…

c240d62

…y/value lists and making the result Dictionary<never> when a list of incorrect depth is supplied.

reubenrybnik added 5 commits July 26, 2020 10:48

Adding tests for single arguments for zip.

3a024c3

Adding comments that help to explain the logic of PairValue.

964317c

Splitting PairValue into two separate types instead of nesting logic.

e5f2601

Putting an underscore in front of the ObjectValue type to help signif…

4baaa97

…y that it's private since unlike most of the types in this file this is really a type that's specifically a helper for the object function which typically shouldn't be used anywhere else.

Removing a portion of the summary comments for zip since they aren't …

cb7551c

…needed anymore.

reubenrybnik commented Jul 26, 2020

View reviewed changes

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

reubenrybnik commented Jul 26, 2020

View reviewed changes

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

jgonggrijp reviewed Jul 26, 2020

View reviewed changes

Infer the pair list type when possible instead of using any

9eb0f64

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

typescript-bot added the The CI failed When GH Actions fails label Jul 27, 2020

Making object into separate overloads.

e1c104f

typescript-bot removed the The CI failed When GH Actions fails label Jul 27, 2020

reubenrybnik mentioned this pull request Jul 27, 2020

[@types/underscore] Collection and Array Tests - SortBy, IndexBy, CountBy, and Invoke #46352

Merged

8 tasks

jgonggrijp suggested changes Jul 27, 2020

View reviewed changes

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

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

Constraining the list of keys to be members of EnumerableKey.

db3c2d5

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

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

jgonggrijp approved these changes Jul 28, 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 28, 2020

typescript-bot merged commit 3db3316 into DefinitelyTyped:master Jul 28, 2020

reubenrybnik mentioned this pull request Aug 6, 2020

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

Closed

10 tasks

Conversation

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

Uh oh!

Uh oh!

typescript-bot commented Jul 24, 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 24, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

typescript-bot commented Jul 24, 2020

Uh oh!

jgonggrijp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

typescript-bot commented Jul 26, 2020

Uh oh!

typescript-bot commented Jul 26, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

typescript-bot commented Jul 27, 2020

Uh oh!

reubenrybnik commented Jul 27, 2020

Uh oh!

jgonggrijp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

typescript-bot commented Jul 27, 2020

Uh oh!

reubenrybnik commented Jul 27, 2020

Uh oh!

reubenrybnik commented Jul 27, 2020

Uh oh!

jgonggrijp left a comment

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented Jul 28, 2020

Uh oh!

reubenrybnik commented Jul 28, 2020

Uh oh!

reubenrybnik commented Jul 28, 2020

Uh oh!

typescript-bot commented Jul 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

reubenrybnik commented Jul 24, 2020 •

edited

Loading

typescript-bot commented Jul 24, 2020 •

edited

Loading