Add pg/lib/type-overrides types.#51175
Add pg/lib/type-overrides types.#51175typescript-bot merged 4 commits intoDefinitelyTyped:masterfrom
Conversation
|
@groner Thank you for submitting this PR! This is a live comment which I will keep updated. 1 package in 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": 51175,
"author": "groner",
"headCommitOid": "f70d5d01353a8906a854fea253a12888de199801",
"lastPushDate": "2021-03-01T16:47:33.000Z",
"lastActivityDate": "2021-03-04T20:01:43.000Z",
"maintainerBlessed": false,
"mergeOfferDate": "2021-03-04T12:21:57.000Z",
"mergeRequestDate": "2021-03-04T20:01:43.000Z",
"mergeRequestUser": "groner",
"hasMergeConflict": false,
"isFirstContribution": false,
"popularityLevel": "Popular",
"pkgInfo": [
{
"name": "pg",
"kind": "edit",
"files": [
{
"path": "types/pg/lib/type-overrides.d.ts",
"kind": "definition"
},
{
"path": "types/pg/pg-tests.ts",
"kind": "test"
}
],
"owners": [
"pspeter3",
"HoldYourWaffle"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Popular"
}
],
"reviews": [
{
"type": "approved",
"reviewer": "HoldYourWaffle",
"date": "2021-03-04T12:21:21.000Z",
"isMaintainer": false
},
{
"type": "stale",
"reviewer": "DanielRosenwasser",
"date": "2021-03-01T09:06:18.000Z",
"abbrOid": "c0ada2a"
}
],
"ciResult": "pass"
} |
|
🔔 @pspeter3 @HoldYourWaffle — 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? Comparison details 📊
It looks like nothing changed too much. I won’t post performance data again unless it gets worse. |
HoldYourWaffle
left a comment
There was a problem hiding this comment.
The content looks good to me, but I'm not sure (read: too dumb) if this should be put in a separate file with a triple-slash directive. I've never seen this before, but I suppose it 'makes sense'.
Could you perhaps explain the reasoning behind this structure?
|
@HoldYourWaffle Thanks for looking at this. I was having some problems with TS2693 ('TypeOverrides' only refers to a type, but is being used as a value here.) which I thought I had resolved by adding the triple-slash directive. Now I'm able to remove the triple-slash directive without breaking the test, so I guess that wasn't it. I tried using I'll remove the triple-slash directive since it seems to be superfluous. I'm open to other suggestions. |
|
@HoldYourWaffle 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? |
|
@groner Unfortunately, this pull request currently has a merge conflict 😥. Please update your PR branch to be up-to-date with respect to master. Have a nice day! |
f02db4e to
c0ada2a
Compare
types/pg/pg-tests.ts
Outdated
| @@ -1,4 +1,5 @@ | |||
| import { types, Client, CustomTypesConfig, QueryArrayConfig, Pool } from 'pg'; | |||
| import TypeOverrides from 'pg/lib/type-overrides'; | |||
There was a problem hiding this comment.
I checked the source of pg again, and if I understand things correctly the reason we need this to be a separate file is because TypeOverrides is not exported from index.js.
Would that be correct?
There was a problem hiding this comment.
Yes, this PR is introducing types that are not currently exported from index.js. Do you think that could be a problem?
There was a problem hiding this comment.
I'm not entirely sure... I think it's unorthodox, but I don't see a reason why it would create actual issues.
Perhaps it's a good idea to let a maintainer look at this, but I don't know of a way to do that other than to wait until the bot does it.
There was a problem hiding this comment.
@RyanCavanaugh Sorry to bother you for this, but your amazing bot seems to fall short here. Is there a way to "request human attention" that I'm unaware of?
There was a problem hiding this comment.
Do you mean reaching out to the pg module maintainers or asking a maintainer of this repo?
There was a problem hiding this comment.
The latter. I'm pretty sure that this describes the module accurately, but I'm not 100% sure that this is the "correct" or "orthodox" way to do things around here.
|
Re-ping @pspeter3, @HoldYourWaffle: This PR has been out for over a week, yet I haven't seen any reviews. Could someone please give it some attention? Thanks! |
|
@groner 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. Thank you! |
|
@DanielRosenwasser, @HoldYourWaffle 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? |
|
@groner Everything looks good here. Great job! I am ready to merge this PR (at f70d5d0) 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! ❤️ (@pspeter3, @HoldYourWaffle: you can do this too.) |
|
@DanielRosenwasser 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? |
|
Ready to merge |
|
I just published |
This PR adds the
TypeOverridetype from pg/lib/type-override.js.npm test <package to test>.If changing an existing definition:
If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.