Pass mention list down to puppet and use mentionIdList from puppet if possible #1662

windmemory · 2018-11-27T03:36:05Z

Refer to issue: #1560

…ossible

huan

Thanks for the improvement!

I have some minor questions for you need to be fixed, and please feel free to let me know if you have any questions.

src/user/message.ts

windmemory · 2018-11-27T06:07:00Z

Updated

huan · 2018-11-27T14:54:29Z

I'll merge this PR after the CI turns green.

windmemory · 2018-11-28T02:37:11Z

I've fixed 4 ci jobs, the 2 remain failed.

Travis CI

The Travic CI build failed in the process of running npm run test:pack, and I saw the error is below:

02:14:56 INFO PuppetManager install(wechaty-puppet-puppeteer@^0.14.1) done
02:15:27 ERR PuppetPuppeteerBridge start() exception: TimeoutError: Navigation Timeout Exceeded: 30000ms exceeded
02:15:27 ERR PuppetPuppeteer initBridge() exception: TimeoutError: Navigation Timeout Exceeded: 30000ms exceeded
02:15:27 WARN PuppetPuppeteerBridge stop() page.close() exception: Error: Protocol error: Connection closed. Most likely the page has been closed.
02:15:27 ERR PuppetPuppeteer start() exception: Error: Error: TimeoutError: Navigation Timeout Exceeded: 30000ms exceeded
Error: Error: Error: TimeoutError: Navigation Timeout Exceeded: 30000ms exceeded
    at PuppetPuppeteer.puppet.on.error (/tmp/npm-pack-testing.6486/node_modules/wechaty/dist/src/wechaty.js:437:44)
    at PuppetPuppeteer.emit (events.js:182:13)
    at PuppetPuppeteer.EventEmitter.emit (domain.js:442:20)
    at PuppetPuppeteer.emit (/tmp/npm-pack-testing.6486/node_modules/wechaty-puppet/dist/src/puppet.js:165:22)
    at PuppetPuppeteer.<anonymous> (/tmp/npm-pack-testing.6486/node_modules/wechaty/node_modules/wechaty-puppet-puppeteer/dist/src/puppet-puppeteer.js:106:22)
    at Generator.throw (<anonymous>)
    at rejected (/tmp/npm-pack-testing.6486/node_modules/wechaty/node_modules/wechaty-puppet-puppeteer/dist/src/puppet-puppeteer.js:23:65)
02:15:27 ERR Wechaty start() exception: Error: Error: TimeoutError: Navigation Timeout Exceeded: 30000ms exceeded
Error: Error: Error: TimeoutError: Navigation Timeout Exceeded: 30000ms exceeded
    at PuppetPuppeteer.puppet.on.error (/tmp/npm-pack-testing.6486/node_modules/wechaty/dist/src/wechaty.js:437:44)
    at PuppetPuppeteer.emit (events.js:182:13)
    at PuppetPuppeteer.EventEmitter.emit (domain.js:442:20)
    at PuppetPuppeteer.emit (/tmp/npm-pack-testing.6486/node_modules/wechaty-puppet/dist/src/puppet.js:165:22)
    at PuppetPuppeteer.<anonymous> (/tmp/npm-pack-testing.6486/node_modules/wechaty/node_modules/wechaty-puppet-puppeteer/dist/src/puppet-puppeteer.js:106:22)
    at Generator.throw (<anonymous>)
    at rejected (/tmp/npm-pack-testing.6486/node_modules/wechaty/node_modules/wechaty-puppet-puppeteer/dist/src/puppet-puppeteer.js:23:65)
02:15:27 INFO Wechaty <wechaty-puppet-mock> stop() v0.23.22 is stoping ...
02:15:27 INFO Wechaty <wechaty-puppet-wechat4u> stop() v0.23.22 is stoping ...
02:15:27 INFO Wechaty <wechaty-puppet-puppeteer> stop() v0.23.22 is stoping ...
02:15:27 WARN PuppetPuppeteerBridge stop() page.close() exception: Error: Protocol error: Connection closed. Most likely the page has been closed.

Since the error is related to wechaty-puppet-puppeteer, I got no clue of how to fix it. Could you please help me on that?

code climate

It complains about:

Function mention has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.

Seems like this issue needs a refactor work on the mention function, which might introduce a lot of changes potentially. And I saw some other improvements could be done for the message.ts file in the codeclimate console. So I would suggest that we create another issue to track this work instead of in this one.

huan · 2018-11-28T04:13:08Z

Ok, please file another issue to track CodeClimate.

And could you please push another commit to see if the Travis CI could pass? Because the puppeteer puppet sometimes will timeout for no reason.

windmemory · 2018-11-28T04:15:34Z

Ok, please file another issue to track CodeClimate.

Sure, will create another issue for the CodeClimate issue.

And could you please push another commit to see if the Travis CI could pass? Because the puppeteer puppet sometimes will timeout for no reason.

No, Travis CI won't pass, I tried to restart the Travis CI job manually, but failed again with same error.

huan · 2018-11-28T04:16:48Z

If it does not work by restarting Travis ci manually, then push another commit might be helpful.

windmemory · 2018-11-28T04:17:19Z

Got it, will do.

windmemory · 2018-11-28T04:22:22Z

Code Climate issue created: #1663

huan · 2018-11-28T04:22:27Z

About the Cognitive Complexity, we can learn from here: https://docs.codeclimate.com/docs/cognitive-complexity

windmemory · 2018-11-28T04:31:15Z

While reviewing another PR, I noticed that I forgot to modify the code in room.ts file... Will do that in a minute.

Please DO NOT merge this PR yet.

huan · 2018-11-28T04:54:00Z

Yes, there's something caused the confusion.

After having a look into the code, I believe there should not be many places to support the mention in say. One and only one method support it should be the best practice.

Then I'd like to do the following change:

class Message {
  // ...
  public async say (	   public async say (
    textOrContactOrFileOrUrl : string | Contact | FileBox | UrlLink,	     textOrContactOrFileOrUrl : string | Contact | FileBox | UrlLink,
-     mention?   : Contact | Contact[],	     mention?   : Contact | Contact[],
  ): Promise<void> {

Remove the mention support in the Message.say, only leave it in the Room.say, because it's room related.

windmemory · 2018-11-28T04:58:15Z

~~Actually I don't quite agree with the design.~~

Well, I think I am actually agree with you, but just think further of this situation.

When I use message.say() I might expect that I am replying this message. So if this happened in a room, I would like to mention the one who said the message. Does this make sense to you?

~~If so, we should have a default mention contact in the message.say() when it happened in a room.~~

We should remove the mention argument in the message.say() function, and pass a default mention contact down to puppet in room situation.

huan · 2018-11-28T05:02:25Z

It's wired: why do I have to mention someone when I say something in a room?

windmemory · 2018-11-28T05:06:34Z

If you call the message.say() function, then you already got a message, and this message is from a room. When you reply back this message, you want to reply to the people who said this message.(I assume)

Imagine that multiple people discuss something in a room, and only someone attracts your attention, and you only want to reply to that message.

windmemory · 2018-11-28T05:07:54Z

I will assume that if you want to say something in a room, you will use room.say() not message.say().

huan · 2018-11-28T05:12:02Z

No, reply is not the case.

say is just you want to say something. it's named as say not a reply because it's not a reply function.

the Message.say is just a convenience method to let you not have to specify the room or contact conversation.

Just say it is enough.

OR

How about let's get rid of the Message.say Entirely? Because it seems to make you a bit confusing, maybe others will too.

Another reason to do this is: it's obvious that a message will not be able to say . (we can say to contact, or say in room, but how can we say XXX message?)

windmemory · 2018-11-28T05:15:54Z

Yep, if that's the definition of message.say(), then we should not add extra logic in it.

We can remove it, or rename it as reply? Message.reply() makes more sense, and give developer convenience to do the reply logic in it.

huan · 2018-11-28T05:22:27Z

It will be a breaking change, so please just remove the mention in the Message.say for now.

And let's talk about it later, because it's not a big deal.

windmemory · 2018-11-28T05:48:59Z

Removed mention in the Message.say and pass the 5 CIs.

huan · 2018-11-28T05:52:56Z

Thanks for the great contribution!

windmemory · 2018-11-28T05:55:01Z

Glad to see Wechaty getting better :)

windmemory · 2018-11-28T05:56:31Z

Just a small reminder, Label related design is still in pending, I am waiting to implement that feature :P

Besides, @xinbenlv is eager to have that feature in Wechaty, so please squeeze a little time review that design~

windmemory added 2 commits November 27, 2018 11:27

pass mention list down and fetch mention list from mentionIdList if p…

0c36e57

…ossible

0.23.22

4ba6d91

huan requested changes Nov 27, 2018

View reviewed changes

src/user/message.ts Outdated Show resolved Hide resolved

src/user/message.ts Outdated Show resolved Hide resolved

fix code style and deep import issue

ae20795

windmemory added 2 commits November 28, 2018 08:47

update wechaty-puppet version to 0.15

f8577c3

fix test

8402661

huan approved these changes Nov 28, 2018

View reviewed changes

minor change to trigger travis ci rebuild

7b10baa

windmemory mentioned this pull request Nov 28, 2018

Refactor message.ts file to fit the requirement in CodeClimate #1663

Closed

pass mention list in room.say function

9081151

windmemory mentioned this pull request Nov 28, 2018

Support real mention function wechaty/wechaty-puppet-padpro#4

Closed

2 tasks

Merge branch 'master' into master

2fafea5

remove mention param from message.say()

f2023a0

huan merged commit ae37198 into wechaty:master Nov 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass mention list down to puppet and use mentionIdList from puppet if possible #1662

Pass mention list down to puppet and use mentionIdList from puppet if possible #1662

windmemory commented Nov 27, 2018

huan left a comment

windmemory commented Nov 27, 2018

huan commented Nov 27, 2018 •

edited

windmemory commented Nov 28, 2018

huan commented Nov 28, 2018 •

edited

windmemory commented Nov 28, 2018 •

edited

huan commented Nov 28, 2018 •

edited

windmemory commented Nov 28, 2018

windmemory commented Nov 28, 2018

huan commented Nov 28, 2018

windmemory commented Nov 28, 2018 •

edited

huan commented Nov 28, 2018 •

edited

windmemory commented Nov 28, 2018 •

edited

huan commented Nov 28, 2018

windmemory commented Nov 28, 2018

windmemory commented Nov 28, 2018

huan commented Nov 28, 2018 •

edited

windmemory commented Nov 28, 2018

huan commented Nov 28, 2018

windmemory commented Nov 28, 2018

huan commented Nov 28, 2018

windmemory commented Nov 28, 2018

windmemory commented Nov 28, 2018

Pass mention list down to puppet and use mentionIdList from puppet if possible #1662

Pass mention list down to puppet and use mentionIdList from puppet if possible #1662

Conversation

windmemory commented Nov 27, 2018

huan left a comment

Choose a reason for hiding this comment

windmemory commented Nov 27, 2018

huan commented Nov 27, 2018 • edited

windmemory commented Nov 28, 2018

Travis CI

code climate

huan commented Nov 28, 2018 • edited

windmemory commented Nov 28, 2018 • edited

huan commented Nov 28, 2018 • edited

windmemory commented Nov 28, 2018

windmemory commented Nov 28, 2018

huan commented Nov 28, 2018

windmemory commented Nov 28, 2018 • edited

huan commented Nov 28, 2018 • edited

windmemory commented Nov 28, 2018 • edited

huan commented Nov 28, 2018

windmemory commented Nov 28, 2018

windmemory commented Nov 28, 2018

huan commented Nov 28, 2018 • edited

OR

windmemory commented Nov 28, 2018

huan commented Nov 28, 2018

windmemory commented Nov 28, 2018

huan commented Nov 28, 2018

windmemory commented Nov 28, 2018

windmemory commented Nov 28, 2018

huan commented Nov 27, 2018 •

edited

huan commented Nov 28, 2018 •

edited

windmemory commented Nov 28, 2018 •

edited

huan commented Nov 28, 2018 •

edited

windmemory commented Nov 28, 2018 •

edited

huan commented Nov 28, 2018 •

edited

windmemory commented Nov 28, 2018 •

edited

huan commented Nov 28, 2018 •

edited