[@types/underscore] Object Tests - MapObject, Pairs, and FindKey by reubenrybnik · Pull Request #46422 · DefinitelyTyped/DefinitelyTyped

reubenrybnik · 2020-07-29T01:33:12Z

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 is an addendum to the original the planned set for #45201 and includes the following changes:

Adds tests for mapObject, pairs, and findKey.
Updates collection and dictionary types and iterators to allow a fallback item type to be specified rather than always falling back to never.
Adds an ObjectIteratee and ObjectIterateeResult type.
Updates mapObject to use ObjectIteratee and ObjectIterateeResult.
Updates pairs to try to figure out what its value element is if it can.
Updates findKey to use ObjectIteratee.
Updates mapObject and findKey in Underscore and _Chain to be more in sync with UnderscoreStatic.

…g tests.

…atic.property.

typescript-bot · 2020-07-29T01:33:46Z

@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": 46422,
  "author": "reubenrybnik",
  "owners": [
    "borisyankov",
    "jbaldwin",
    "ccurrens",
    "confususs",
    "jgonggrijp",
    "ffflorian",
    "regevbr",
    "peterblazejewicz",
    "reubenrybnik"
  ],
  "dangerLevel": "ScopedAndTested",
  "headCommitAbbrOid": "1b03f4d",
  "headCommitOid": "1b03f4d13078717e6c797e315c34517ad13d86e7",
  "mergeIsRequested": true,
  "stalenessInDays": 0,
  "lastPushDate": "2020-07-31T17:29:27.000Z",
  "lastCommentDate": "2020-07-31T23:44:51.000Z",
  "maintainerBlessed": false,
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/46422/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-31T22:53:23.000Z",
  "reviewersWithStaleReviews": [],
  "approvalFlags": 2,
  "isChangesRequested": false
}

typescript-bot · 2020-07-29T01:33:47Z

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

reubenrybnik · 2020-07-29T01:34:55Z

As a quick disclaimer here, my company's solution does not use mapObject or pairs at all in TS code and only uses findKey once, so testing the change in my own code doesn't do much in this case.

reubenrybnik · 2020-07-29T01:39:47Z

Well, I can still take the test code and verify that it does the things I expect in JS at least.

types/underscore/underscore-tests.ts

types/underscore/index.d.ts

typescript-bot · 2020-07-29T03:58:11Z

👋 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	#46422	diff
Batch compilation
Memory usage (MiB)	66.4	60.1	-9.5%
Type count	12952	13446	+4%
Assignability cache size	1521	1632	+7%

Language service
Samples taken	7436	7766	+4%
Identifiers in tests	7436	7766	+4%
`getCompletionsAtPosition`
Mean duration (ms)	176.6	177.5	+0.5%
Mean CV	9.2%	9.4%
Worst duration (ms)	335.8	377.1	+12.3%
Worst identifier	_	resultUnionPartialMemoIterator
`getQuickInfoAtPosition`
Mean duration (ms)	173.0	173.9	+0.5%
Mean CV	9.4%	9.6%	+2.7%
Worst duration (ms)	337.7	359.4	+6.4%
Worst identifier	reduce	extractUnderscoreTypes

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

types/underscore/index.d.ts

jgonggrijp

This review seems familiar, somehow...

As always, I should acknowledge that your changes are already an improvement over the old situation!

types/underscore/index.d.ts

types/underscore/underscore-tests.ts

typescript-bot · 2020-07-30T21:18:07Z

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

…IterateeResult.

…of _.object (and to refer to _.object at all in the case of Underscore and _Chain).

typescript-bot · 2020-07-30T23:32:22Z

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

…t that resolved to Partial<any> in the non-collection case now, which is probably more or less object (though it might be worth converting that to object if that's not the case).

reubenrybnik · 2020-07-30T23:37:04Z

As a quick point of interest, I've found that Partial<any> only allows objects to be assigned to it, so it is an acceptable type to end up with if necessary when one wants to make sure that a provided value is an object.

types/underscore/index.d.ts

…s I can.

…re objects and technically work.

types/underscore/underscore-tests.ts

…ating iteratee results.

reubenrybnik · 2020-07-31T05:23:12Z

Thanks @jgonggrijp, it's much nicer to have the iteratee types consolidated and to be providing more specific results for findKey. I think this is ready for another look whenever you have a moment.

jgonggrijp

This is good stuff!

types/underscore/index.d.ts

@jgonggrijp

…in paris as recommended by @jgonggrijp.

jgonggrijp

Excellent.

reubenrybnik · 2020-07-31T23:44:08Z

Awesome @jgonggrijp, hope you're having a great time! Let me know if you'd like me to hold off on further PRs for a bit 😄

reubenrybnik · 2020-07-31T23:44:35Z

Also thanks for the extra-excellent review on this one!

reubenrybnik · 2020-07-31T23:44:51Z

Ready to merge

typescript-bot · 2020-07-31T23:49:10Z

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

jgonggrijp · 2020-08-01T07:35:27Z

My pleasure, @reubenrybnik! You don't need to hold off for me, but feel free to do it anyway, of course.

Since this appears to be a natural moment for a break in your series, I'd like to briefly mention my Patreon page. I believe that improving what is already there, rather than creating something new from scratch, is the best way to move forward as a planet. This is why I contribute to open source software, including reviews like these. While I will never stop contributing, receiving more donations would help me dedicate more time to the common good. If you, your company, or anyone else reading this would be willing to make a pledge, that would be great.

https://www.patreon.com/juliangonggrijp

@reubenrybnik

…apObject, Pairs, and FindKey by @reubenrybnik * Updating type definitions for mapObject, pairs, and findKey and adding tests. * Also updating Underscore.property to match the update to UnderscoreStatic.property. * Adding a few more tests. * Combining Iteratee with ObjectIteratee and IterateeResult with ObjectIterateeResult. * Improving the condition for choosing between Partial<T> and object. * Fixing a test that breaks slightly with iteratee changes. * Updating findKey to result in keyof V rather than just string. * Updating the documentation of pairs to refer to a specific signature of _.object (and to refer to _.object at all in the case of Underscore and _Chain). * Dropping object and just going with Partial<T> because I realized that that resolved to Partial<any> in the non-collection case now, which is probably more or less object (though it might be worth converting that to object if that's not the case). * Handling the possibility of a property that is a type union as well as I can. * Switching constraints from dictionaries to collections since arrays are objects and technically work. * Adding a comment above the assignability test. * Defaulting to a collection type of any for non-collections when evaluating iteratee results. * Using Extract instead of making up a new type and also using keyof V in paris as recommended by @jgonggrijp.

reubenrybnik added 2 commits July 28, 2020 18:13

Updating type definitions for mapObject, pairs, and findKey and addin…

3530466

…g tests.

Also updating Underscore.property to match the update to UnderscoreSt…

ae409ac

…atic.property.

reubenrybnik requested a review from borisyankov as a code owner July 29, 2020 01:33

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 29, 2020

Adding a few more tests.

2370a3c

reubenrybnik commented Jul 29, 2020

View reviewed changes

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

reubenrybnik commented Jul 29, 2020

View reviewed changes

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

reubenrybnik commented Jul 29, 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 29, 2020

reubenrybnik commented Jul 29, 2020

View reviewed changes

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

jgonggrijp suggested changes Jul 30, 2020

View reviewed changes

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

reubenrybnik added 5 commits July 30, 2020 15:58

Combining Iteratee with ObjectIteratee and IterateeResult with Object…

904f379

…IterateeResult.

Improving the condition for choosing between Partial<T> and object.

1ce562f

Fixing a test that breaks slightly with iteratee changes.

3fedc41

Updating findKey to result in keyof V rather than just string.

ad45cae

Updating the documentation of pairs to refer to a specific signature …

4c52b61

…of _.object (and to refer to _.object at all in the case of Underscore and _Chain).

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

Dropping object and just going with Partial<T> because I realized tha…

425aadf

…t that resolved to Partial<any> in the non-collection case now, which is probably more or less object (though it might be worth converting that to object if that's not the case).

reubenrybnik commented Jul 30, 2020

View reviewed changes

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

Handling the possibility of a property that is a type union as well a…

b3240f4

…s I can.

Switching constraints from dictionaries to collections since arrays a…

f627f08

…re objects and technically work.

reubenrybnik commented Jul 31, 2020

View reviewed changes

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

reubenrybnik added 2 commits July 30, 2020 17:30

Adding a comment above the assignability test.

6ab2e41

Defaulting to a collection type of any for non-collections when evalu…

4437241

…ating iteratee results.

jgonggrijp reviewed Jul 31, 2020

View reviewed changes

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

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

Using Extract instead of making up a new type and also using keyof V …

1b03f4d

…in paris as recommended by @jgonggrijp.

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

typescript-bot merged commit 7032a7e into DefinitelyTyped:master Jul 31, 2020

Conversation

reubenrybnik commented Jul 29, 2020

Uh oh!

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

Uh oh!

reubenrybnik commented Jul 29, 2020

Uh oh!

reubenrybnik commented Jul 29, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

typescript-bot commented Jul 29, 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!

typescript-bot commented Jul 30, 2020

Uh oh!

typescript-bot commented Jul 30, 2020

Uh oh!

reubenrybnik commented Jul 30, 2020

Uh oh!

Uh oh!

Uh oh!

reubenrybnik commented Jul 31, 2020

Uh oh!

jgonggrijp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jgonggrijp left a comment

Choose a reason for hiding this comment

Uh oh!

reubenrybnik commented Jul 31, 2020

Uh oh!

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

Uh oh!

Uh oh!

reubenrybnik commented Jul 31, 2020

Uh oh!

typescript-bot commented Jul 31, 2020

Uh oh!

jgonggrijp commented Aug 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

typescript-bot commented Jul 29, 2020 •

edited

Loading

reubenrybnik commented Jul 31, 2020 •

edited

Loading