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.

Sign up for GitHub

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

Jump to bottom

fix: room mention contact should using roomAlias https://github.com/Chatie/wechaty/issues/1604 #1607

Merged
merged 5 commits into from
Oct 3, 2018
Merged

fix: room mention contact should using roomAlias https://github.com/Chatie/wechaty/issues/1604 #1607

merged 5 commits into from
Oct 3, 2018

Conversation

kis87988
Copy link
Contributor

This is a PR continue with #1605 with clean commit

I'm submitting a...

[ ] Bug Fix
[x] Feature
[ ] Other (Refactoring, Added tests, Documentation, ...)

Checklist

  • Commit Messages follow the Conventional Commits pattern
    • A feature commit message is prefixed "feat:"
    • A bugfix commit message is prefixed "fix:"
  • Tests for the changes have been added

Description

please describe the changes that you are making

for features, please describe how to use the new feature

please include a reference to an existing issue, if applicable

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@kis87988 kis87988 mentioned this pull request Sep 28, 2018
Closed
2 tasks
huan
huan requested changes Sep 28, 2018
View reviewed changes
Copy link
Member

@huan huan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please think deep and read my reviews carefully.

src/user/room.ts Outdated
@@ -428,23 +428,33 @@ export class Room extends Accessory implements Sayable {
textOrContactOrFileOrUrl : string | Contact | FileBox | UrlLink,
mention? : Contact | Contact[],
): Promise<void> {

const replyToList: Contact[] = [].concat(mention as any || [])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use [...mention] should be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but the replyToList is original code, and what if mention is undefine?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[...(undefined||[])]

src/user/room.ts Outdated

const replyToList: Contact[] = [].concat(mention as any || [])

const mentionAliasPromiseList = replyToList.length > 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe replyToList.length > 0 is not necessary at here.

Rename mentionAliasPromiseList to mentionAliasList because it's not a promise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mentionAliasPromiseList may contain string | null, mentionAliasList should only contain string. Therefore, I think the line 440 is the one being filter.
However, you are right. I should give a good name for the one contain string | null

src/user/room.ts Outdated
: mention ? mention.name() : '',
)
let text: string
mentionAliasList.length > 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mentionAliasList.length > 1 is not necessary at here.

huan
huan requested changes Oct 2, 2018
View reviewed changes
Copy link
Member

@huan huan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow my review and change those line as required.

I'll merge this PR after that, thanks!

src/user/room.ts
@@ -428,23 +428,31 @@ export class Room extends Accessory implements Sayable {
textOrContactOrFileOrUrl : string | Contact | FileBox | UrlLink,
mention? : Contact | Contact[],
): Promise<void> {

const replyToList: Contact[] = Array.isArray(mention)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change this line to:

const replyToList: Contact[] = [...(mention || [])]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can not because the mention may be Contact, so I need to check mention is an Contact[] or not.
Otherwise,
image

Copy link
Member

@huan huan Oct 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right.

Then could we use [].concat(mention || []) instead? Because I want to here be as simple as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that works because the mention may be Contact, and I believe we don't want to have list of list(2D array) here
image

src/user/room.ts
? [...mention]
: mention ? [mention] : []

const mentionAliasPromiseList = await Promise.all(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename mentionAliasPromiseList to mentionAliasList

Copy link
Contributor Author

@kis87988 kis87988 Oct 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since below discussion, we should give a better name since that is await.
However, mentionAliasList contain string | null base on Promise.all() fail will return null.
Therefore, I suggest name should be mentionAliasListResult or mentionAliasListPromiseResult, but the second one name may be too long..

src/user/room.ts
replyToList
.map(c => this.alias(c) || c.name()))

const mentionAliasList: string[] = mentionAliasPromiseList
Copy link
Member

@huan huan Oct 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line can be removed because the array should not include the null anymore after the above code this.alias(c) || c.name()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but it is possible promise fail and got null value for c.name(), isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before rename:
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c.name() will always return a string, so it should never be a null.

However, you can keep this code and I'll have a look after we merged it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok.

@huan huan merged commit a2b055e into wechaty:master Oct 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@huan huan huan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kis87988 @huan