Add types for @mysql/xdevapi #28062
Add types for @mysql/xdevapi #28062
Conversation
typescript-bot
added
the
New Definition
|
@r-a303931 Thank you for submitting this PR! Because this is a new definition, a DefinitelyTyped maintainer will be reviewing this PR in the next few days once the Travis CI build passes. In the meantime, if the build fails or a merge conflict occurs, I'll let you know. Have a nice day! |
types/mysql__xdevapi/index.d.ts
Outdated
| @@ -0,0 +1,493 @@ | |||
| // Type definitions for @mysql/xdevapi 8.0 | |||
| // Project: https://github.com/mysql/mysql-connector-nodejs (Does not have to be to GitHub, but prefer linking to a source code repository rather than to a project website.) | |||
There was a problem hiding this comment.
It should be just // Project: https://github.com/mysql/mysql-connector-nodejs here
typescript-bot
moved this from Review
to Needs Author Attention
in Pull Request Status Board
typescript-bot
added
Revision needed
|
@r-a303931 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! |
typescript-bot
added
the
Revision needed
|
🔔 @plantain-00 - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate? |
typescript-bot
added
the
Revision needed
typescript-bot
moved this from Needs Author Attention
to Review
in Pull Request Status Board
typescript-bot
added
Other Approved
|
|
||
| export interface SocketFactory { | ||
| // tslint:disable-next-line | ||
| createSocket: Function; |
There was a problem hiding this comment.
As discussed here, it's not good practice to use Function.
| } | ||
|
|
||
| export class Schema implements DatabaseObject { | ||
| // tslint:disable-next-line |
There was a problem hiding this comment.
What TSLint rule are you disabling here and why?
| createCollection<T>( | ||
| name: string, | ||
| options: CreateCollectionOptions | ||
| // tslint:disable-next-line |
| arrayAppend< | ||
| K extends keyof T = keyof T, | ||
| // tslint:disable-next-line:no-unnecessary-generics | ||
| J extends keyof T[K] = keyof T[K] |
There was a problem hiding this comment.
I don't think you need these defaults here (e.g. = keyof T).
In fact, you don't even need J; you can just index with keyof T[K].
| getClassName(): string; | ||
|
|
||
| // tslint:disable-next-line | ||
| getCollection<T>(name: string): Collection<T>; |
There was a problem hiding this comment.
As discussed here, it's cleaner if this is just getCollection(name: string): Collection<any>.
| getName(): string; | ||
|
|
||
| // tslint:disable-next-line:no-unnecessary-generics | ||
| getTable<T = {}>(name: string): Table<T>; |
There was a problem hiding this comment.
Same here, except Table<{}>.
typescript-bot
moved this from Review
to Needs Author Attention
in Pull Request Status Board
typescript-bot
added
Revision needed
typescript-bot
added
Abandoned
|
@r-a303931 To keep things tidy, we have to close PRs that aren't mergeable but don't have activity from their author. No worries, though - please open a new PR if you'd like to continue with this change. Thank you! |
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(ortscif notslint.jsonis present).The package does not already provide its own types, or cannot have its
.d.tsfiles generated via--declarationIf this is for an NPM package, match the name. If not, do not conflict with the name of an NPM package.
Create it with
dts-gen --dt, not by basing it on an existing project.tslint.jsonshould be present, andtsconfig.jsonshould havenoImplicitAny,noImplicitThis,strictNullChecks, andstrictFunctionTypesset totrue.