Skip to content
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

room mention contact should using roomAlias #1604

Closed
kis87988 opened this issue Sep 26, 2018 · 5 comments
Closed

room mention contact should using roomAlias #1604

kis87988 opened this issue Sep 26, 2018 · 5 comments

Comments

@kis87988
Copy link
Contributor

Purpose:
For now,
Our room.say(content,contact) will call contact name, but if contact has room alias, it should do room alias.
the order should be roomAlias > name. If the contact not set roomAlias, it would be name.
Is this a good idea? if so, I can do this PR

@huan
Copy link
Member

huan commented Sep 27, 2018

Yes, the idea is the right direction that we should go.

PR is welcome.

kis87988 added a commit to kis87988/Chatie-wechaty that referenced this issue Sep 28, 2018
huan pushed a commit that referenced this issue Oct 3, 2018
* fix: change room say function mention contact roomAlias not name
refer issue: #1604

* change getRoomAlias() to promise.all() and remove getRoomAlias()

* re-deal with null promise array

* 0.23.14

* clean code
@huan
Copy link
Member

huan commented Oct 3, 2018

PR merged.

Thanks for your contribution!

@huan
Copy link
Member

huan commented Oct 3, 2018

Your code has a bug with the alias(), I had fixed in the latest commit.

In short: alias() will return a Promise that you should await, or you will never get the chance to call name()

- this.alias(c) || c.name()
+ await this.alias(c) || c.name()

And that's the reason why your typing is not right. Instead of checking it again for the real reason, you go to the wrong direction one more step: force casting the type, which should normally be avoided.

Please have a look at the new code, and feel free to ask if you have more questions.

Thanks!

@kis87988
Copy link
Contributor Author

kis87988 commented Oct 3, 2018

I see! That is why.
Thank you. I have learned a lesson 👍

@huan
Copy link
Member

huan commented Oct 4, 2018

We had added an additional check for this special case: we do not allow to use promise as boolean anymore.

@lijiarui Maybe you will want this feature too.

ERROR: /Users/zixia/chatie/wechaty/src/user/room.ts[437, 52]: Promises should be awaited when used in boolean expressions

See: microsoft/tslint-microsoft-contrib#174 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants