New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade types for uuid@8.3.0 #46395
Upgrade types for uuid@8.3.0 #46395
Conversation
@ctavan Thank you for submitting this PR! This is a live comment which I will keep updated. Code ReviewsBecause this is a widely-used package, a DT maintainer will need to review it before it can be merged. 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": 46395,
"author": "ctavan",
"owners": [
"iamolivinius",
"felipeochoa",
"cjbarth",
"LinusU",
"ctavan"
],
"dangerLevel": "ScopedAndTested",
"headCommitAbbrOid": "d044ae5",
"headCommitOid": "d044ae595df707da86d75a05ce3555a42df61e28",
"mergeIsRequested": true,
"stalenessInDays": 0,
"lastPushDate": "2020-08-07T07:26:35.000Z",
"lastCommentDate": "2020-08-12T05:51:56.000Z",
"maintainerBlessed": false,
"reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/46395/files",
"hasMergeConflict": false,
"authorIsOwner": true,
"isFirstContribution": false,
"popularityLevel": "Critical",
"anyPackageIsNew": false,
"packages": [
"uuid"
],
"files": [
{
"path": "types/uuid/index.d.ts",
"kind": "definition",
"package": "uuid"
},
{
"path": "types/uuid/interfaces.d.ts",
"kind": "definition",
"package": "uuid"
},
{
"path": "types/uuid/uuid-tests.ts",
"kind": "test",
"package": "uuid"
}
],
"hasDismissedReview": false,
"ciResult": "pass",
"lastReviewDate": "2020-08-11T23:07:58.000Z",
"reviewersWithStaleReviews": [
{
"reviewedAbbrOid": "cd9f7a1",
"reviewer": "rbuckton",
"date": "2020-08-05T19:04:33Z"
},
{
"reviewedAbbrOid": "d87e8c8",
"reviewer": "RyanCavanaugh",
"date": "2020-07-28T15:48:09Z"
}
],
"approvalFlags": 4,
"isChangesRequested": false
} |
🔔 @iamolivinius @felipeochoa @cjbarth @LinusU — please review this PR in the next few days. Be sure to explicitly select |
@ctavan The CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
291bd8c
to
d87e8c8
Compare
👋 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. |
types/uuid/interfaces.d.ts
Outdated
|
||
export type NIL = string; | ||
|
||
export enum Version { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This object doesn't exist at runtime; it should not be an enum. It'd actually be nice to remove this file entirely and fold it into the parent file, since there's no uuid/interfaces
module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, you are right about both, the enum and inlining the interface definitions (I inherited that structure and – being rather new to TypeScript typings – never questioned it).
If I inline uuid/interfaces
how do I go about only export
ing the named exports that are actually exported by the uuid
module? I'm getting
All declarations in this module are exported automatically. Prefer to explicitly write 'export' for clarity. If you have a good reason not to export this declaration, add 'export {}' to the module to shut off automatic exporting.
errors if I don't export all types. I've seen https://stackoverflow.com/a/51970377/1053532 but what is preferred in DefinitelyTyped? Adding export {}
or declaring a namespace
?
Pointing me to a module where this is done correctly would be enough, I can then make the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RyanCavanaugh friendly ping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I went for the export {}
route.
@ctavan 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! |
d87e8c8
to
93de853
Compare
@RyanCavanaugh 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? |
I have inlined the type definitions into It would be great to prefer this PR over #46477 since the other PR contains some subtle differences with respect to return types that are not in line with how the authors of |
b3223c3
to
cd9f7a1
Compare
Just resolved merge conflicts that were introduced due to #46557 |
@rbuckton could you have a look at this one? |
@ctavan 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! |
cd9f7a1
to
88927e7
Compare
@rbuckton, @RyanCavanaugh 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? |
CI failure seems unrelated to this changeset. |
@ctavan The CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
ced9798
to
d044ae5
Compare
@ctavan 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! ❤️ (@iamolivinius, @felipeochoa, @cjbarth, @LinusU: you can do this too.) |
Ready to merge |
* Revert "added typing for "validate" function added in uuid release 8.3.0 (DefinitelyTyped#46557)" This reverts commit c50f120. * Upgrade types for uuid@8.3.0 * Export v1/v4 options
* Revert "added typing for "validate" function added in uuid release 8.3.0 (DefinitelyTyped#46557)" This reverts commit c50f120. * Upgrade types for uuid@8.3.0 * Export v1/v4 options
Please fill in this template.
npm test
.)npm run lint package-name
(ortsc
if notslint.json
is present).Select one of these and delete the others:
If changing an existing definition:
tslint.json
containing{ "extends": "dtslint/dt.json" }
. If for reason the any rule need to be disabled, disable it for that line using// tslint:disable-next-line [ruleName]
and not for whole package so that the need for disabling can be reviewed.Closes #46477.