-
Notifications
You must be signed in to change notification settings - Fork 1k
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
typings v2 #304
base: master
Are you sure you want to change the base?
typings v2 #304
Conversation
Wow, this is a model pull request. You have thought of everything. Give me some time to go over it, I am still new to the world of TypeScript. |
thanks :)
ᐧ
…On Wed, Aug 23, 2017 at 2:34 PM, Tal Ater ***@***.***> wrote:
Wow, this is a model pull request. You have thought of everything.
Give me some time to go over it, I am still new to the world of TypeScript.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#304 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAE_vcySWS_Ioe0cQ1_mR0BpAcluXlnIks5sbIy6gaJpZM4PAS8C>
.
|
types/index.d.ts
Outdated
* | ||
* @param {string[]} command | ||
*/ | ||
removeCommands(command: string[]): void; |
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.
Does removeCommands()
need to be repeated twice?
Could we do something like:
removeCommands(command?: string | string[]): void;
I am unsure of the syntax, so consider my suggestion pseudo code.
types/index.d.ts
Outdated
'errorPermissionBlocked' | | ||
'errorPermissionDenied'; | ||
|
||
export interface Annyang { |
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.
Is export interface the correct way to do this?
Looking at this example, and at the footnote Preventing Name Conflicts, it looks like we might actually want to define a namespace annyang.
Am I understanding this wrong?
- fix typo in contributing file
good points on both. now fixed. |
So will this be merged? Would hate to have my pull request go to waste... |
Definitely! I am just not very experienced with TypeScript, and so asking for help with making sure I don't miss anything. My apologies for it taking so long! |
This would be useful for me. |
Bundling types as discussed in #197.
Description
Motivation and Context
So that annyang can be easily used in typescript apps. See #197.
How Has This Been Tested?
Types of changes
Checklist: