add interface to provide update user info #15
Conversation
src/puppet.spec.ts
Outdated
| @@ -69,6 +69,9 @@ class PuppetTest extends Puppet { | |||
| public async contactList () : Promise<string[]> { return {} as any } | |||
| public async contactQrcode (contactId: string) : Promise<string> { return { contactId } as any } | |||
|
|
|||
| public async contactSelfName (newName: string) : Promise<boolean> { return true } | |||
There was a problem hiding this comment.
We should follow the current naming style, like we used contactQrcode instead of contactSelfQrcode
There was a problem hiding this comment.
I would argue on this issue. Since getting qrcode, modifying name or signature, these operations are only allowed to be done on ContactSelf, so it makes sense to have self inside of the function name to indicate that this function is only related to self, not every contact. Does this make sense to you?
There was a problem hiding this comment.
If they are ONLY allowed to be done on Self, why we need to point this out again?
There was a problem hiding this comment.
When we do coding ourselves, or some other contributors who do not familiar with the logic in wechaty, people will make mistake on that, so name it more specifically could avoid unnecessary mistake. Also save your time for explaining how the function works to the ones who don't know the logic.
src/puppet.spec.ts
Outdated
| @@ -69,6 +69,9 @@ class PuppetTest extends Puppet { | |||
| public async contactList () : Promise<string[]> { return {} as any } | |||
| public async contactQrcode (contactId: string) : Promise<string> { return { contactId } as any } | |||
|
|
|||
| public async contactSelfName (newName: string) : Promise<boolean> { return true } | |||
| public async contactSelfSignature (signature: string) : Promise<boolean> { return true } | |||
There was a problem hiding this comment.
We should follow the current return type like we always used Promise<void> instead of others.
boolean does not make sense because if it fails, there must be an Error, and it should be thrown out instead of return a false.
There was a problem hiding this comment.
Will change this to Promise<void>
There was a problem hiding this comment.
Ok, thanks.
BTW: does that mean you agree with me about this design: it should throw an exception instead of return false?
There was a problem hiding this comment.
Yes, I agree with the design for updating actions: no return value, when fail, exception is expected.
|
Changes made to the function type, and please see my comments related to the naming issue. |
|
After thinking more, I decided to use And there also introduced a breaking change, please see the code for details. |
No description provided.