[@types/underscore] Collection and Array Tests - Type Extractors#45717
Conversation
…rscore and chain functions to a single overload.
…tely instead of using Collection since using the latter behaves oddly with JQuery objects.
|
@reubenrybnik Thank you for submitting this PR! Code ReviewsBecause you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it. Status
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": 45717,
"author": "reubenrybnik",
"owners": [
"borisyankov",
"jbaldwin",
"ccurrens",
"confususs",
"jgonggrijp",
"ffflorian",
"regevbr",
"peterblazejewicz",
"reubenrybnik"
],
"dangerLevel": "ScopedAndTested",
"headCommitAbbrOid": "d67689f",
"headCommitOid": "d67689f4a1eaa3c4c7fe9f6615db6aecb1824e24",
"mergeIsRequested": true,
"stalenessInDays": 0,
"lastCommitDate": "2020-06-27T07:34:26.000Z",
"lastCommentDate": "2020-06-28T02:07:34.000Z",
"reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/45717/files",
"hasMergeConflict": false,
"authorIsOwner": true,
"isFirstContribution": false,
"popularityLevel": "Popular",
"anyPackageIsNew": false,
"packages": [
"underscore"
],
"files": [
{
"filePath": "types/underscore/index.d.ts",
"kind": "definition",
"package": "underscore"
},
{
"filePath": "types/underscore/underscore-tests.ts",
"kind": "test",
"package": "underscore"
}
],
"hasDismissedReview": false,
"ciResult": "pass",
"lastReviewDate": "2020-06-28T00:43:48.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 |
|
👋 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?
Looks like there were a couple significant differences—take a look at worst-case duration for getting completions at a position and worst-case duration for getting quick info at a position to make sure everything looks ok. |
|
Sorry performance benchmarking check, but I don't understand what you're unhappy about, and since one of the things you're complaining about is a test helper function I'm not convinced that you're telling me anything useful about what users will experience so I'm not super motivated to do anything about what you're reporting. |
|
Updated numbers for you here from 589517c. Comparison details 📊
Looks like there were a couple significant differences—take a look at worst-case duration for getting quick info at a position to make sure everything looks ok. |
jgonggrijp
left a comment
There was a problem hiding this comment.
I agree with these changes.
A soft request for adjustment, however: please reduce the length of names in the tests. While I understand that a name like stronglyKeyedSimpleStringObjectDictionary is meant to be self-documenting, the brain strain required to parse such a long name makes me just grab for the search function to look up the definition of the symbol instead, so you may as well have called it banana (not meant as a concrete suggestion). Adding to that, it takes up a lot of space and it makes it harder to get a bird's-eye view of the code as a result. All in all, it hurts the readability of the code instead of improving it.
I suggest concatenating at most three words as a rule of thumb, preferably fewer. Perhaps there are common combinations of words that can be replaced by a single, more expressive word. For example, I would suggest explicit instead of stronglyKeyed and Record instead of SimpleStringObject. It may also not always be necessary to give a complete description of a symbol in its name.
|
@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? |
|
Thanks for the feedback @jgonggrijp! In particular, I definitely went a little crazy unnecessarily calling everything I admittedly didn't end up dropping everything to three words or less like you were hoping, but I dropped some words that were not providing useful information and simplified some phrases. |
|
Updated numbers for you here from 3c29495. Comparison details 📊
Looks like there were a couple significant differences—take a look at worst-case duration for getting completions at a position and worst-case duration for getting quick info at a position to make sure everything looks ok. |
|
Still an improvement, works for me. 👍 |
|
Sounds good @jgonggrijp, thanks! Unfortunately per the below screenshot of typescript-bot's comment around merging I think you need to approve the most recent commit for me to be able to merge this; if you wouldn't mind doing that whenever you have a chance, I'd appreciate it!
It looks like there might be a button to more thoroughly request your reapproval, too, let me try it quick... |
|
There's a button I've tried before, but as far as I can tell clicking it doesn't do anything, which seems odd. Sorry if I caused multiple emails to be sent to you by clicking on it @jgonggrijp.
|
|
@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:
and I'll merge this PR almost instantly. Thanks for helping out! ❤️ |
|
Ready to merge |
…rray Tests - Type Extractors by @reubenrybnik * Adding type extractors and updating existing tests to use them. * Improving handling of mixed-iterability types by simplifying the Underscore and chain functions to a single overload. * Updating TypeOfCollection to check against List and Dictionary separately instead of using Collection since using the latter behaves oddly with JQuery objects. * Renaming a variable to something slightly more friendly. * Renaming variables in tests.
…rray Tests - Type Extractors by @reubenrybnik * Adding type extractors and updating existing tests to use them. * Improving handling of mixed-iterability types by simplifying the Underscore and chain functions to a single overload. * Updating TypeOfCollection to check against List and Dictionary separately instead of using Collection since using the latter behaves oddly with JQuery objects. * Renaming a variable to something slightly more friendly. * Renaming variables in tests.
npm test.)npm run lint package-name(ortscif notslint.jsonis present).This PR continues the planned set that will together add up to #45201 and includes the following changes:
UnderscoreStaticandUnderscoreStatic.chainfunction result evaluation in test code that will hopefully help keep future changes to underlying Underscore types from requiring large swaths of changes to tests.UnderscoreStaticandUnderscoreStatic.chainfunctions in a way that makes them better at handling types with mixed iterability (e.g.number | number[]) which ideally should provide wrapper results that support calls to iterative functions.TypeOfCollectionto check againstListandDictionaryseparately rather than checking againstCollectionsince inference using the latter type interacts very unpleasantly withJQueryobjects for some reason in a way that inference from theListandDictionarytypes alone do not replicate.