load all memberList #275

lijiarui · 2017-02-22T15:27:16Z

#273

huan

No, I don't think this can fix anything.

lijiarui · 2017-02-22T15:42:12Z

Then, what's your suggestion?

huan · 2017-02-22T15:43:31Z

My suggestion is: keep thinking hard, instead of keep asking.

huan · 2017-02-22T17:40:57Z

The best practice of making Pull Request is to keep improving it instead of closing it because close it will lose all the review information.

huan · 2017-02-28T03:39:23Z

src/room.ts

-  nameMap:    Map<string, string>,
-  aliasMap:   Map<string, string>,
+  memberList?: Contact[],
+  nameMap?:    Map<string, string>,


Why I said "right" is because you starting to notice the right problem.

But what you did modify here is not correct.

the memberList, nameMap and aliasMap should not defined by ?, which means they should always not to be undefined or null.

If there's no data for them, they should be empty array, or a empty {}.

huan · 2017-02-28T03:44:01Z

src/room.ts

@@ -130,11 +130,17 @@ export class Room extends EventEmitter implements Sayable {
      const data = await contactGetter(this.id)
      log.silly('Room', `contactGetter(${this.id}) resolved`)
      this.rawObj = data
+      if (!this.rawObj.MemberList) {
+         return this.ready()


You should not do this because this will cause very bad problems.

Oh, sorry, I added before and forgot to delete it.......

huan · 2017-02-28T03:45:07Z

src/room.ts

      await this.readyAllMembers(this.rawObj.MemberList)
      this.obj    = this.parse(this.rawObj)
      if (!this.obj) {
        throw new Error('no this.obj set after contactGetter')
      }
+      if (!this.obj.memberList) {
+        throw new Error('no this.obj.memberLit set after contactGetter')


Should not throw here.

memberList is empty should be treated a normal situation, because the data is async.

If this.rawObj.MemberList is undefined, we should init obj.memberList with [].

huan · 2017-02-28T03:46:24Z

src/room.ts


    return {
      id:         rawObj.UserName,
      encryId:    rawObj.EncryChatRoomId, // ???
      topic:      rawObj.NickName,
      ownerUin:   rawObj.OwnerUin,
-
-      memberList,


If this.rawObj.MemberList is undefined, we should init memberList with [].

coveralls · 2017-02-28T04:19:54Z

Changes Unknown when pulling 2f11cc1 on lijiarui:273 into ** on wechaty:master**.

coveralls · 2017-02-28T05:31:54Z

Changes Unknown when pulling c7e7cf3 on lijiarui:273 into ** on wechaty:master**.

huan

Please rethink the design.

huan · 2017-02-28T07:44:33Z

src/room.ts

-  nameMap:    Map<string, string>,
-  aliasMap:   Map<string, string>,
+  memberList?: Contact[],
+  nameMap?:    Map<string, string>,


Why I said "right" is because you starting to notice the right problem.

But what you did modify here is not correct.

the memberList, nameMap and aliasMap should not defined by ?, which means they should always not to be undefined or null.

If there's no data for them, they should be empty array, or a empty {}.

huan · 2017-02-28T07:46:51Z

src/room.ts

@@ -130,11 +130,17 @@ export class Room extends EventEmitter implements Sayable {
      const data = await contactGetter(this.id)
      log.silly('Room', `contactGetter(${this.id}) resolved`)
      this.rawObj = data
+      if (!this.rawObj.MemberList) {
+        this.rawObj.MemberList = []


Do not modify rawObj, because this data comes from Browser, which should be treated as immutable.

Do this job on this.obj instead.

huan · 2017-02-28T07:49:25Z

src/room.ts

      await this.readyAllMembers(this.rawObj.MemberList)
      this.obj    = this.parse(this.rawObj)
      if (!this.obj) {
        throw new Error('no this.obj set after contactGetter')
      }
+      if (!this.obj.memberList) {
+        this.obj.memberList = []


Do not do this job here.

Do this job inside this.parse instead.

huan · 2017-02-28T07:52:52Z

src/room.ts

@@ -204,6 +210,16 @@ export class Room extends EventEmitter implements Sayable {
      return null
    }

+    if (!rawObj.MemberList) {


Get rid of this if, and also get rid of return inside it.

We should prevent to use multiple return statement for the final data.

Only return the final data at the end of function statements. The "return"s before that should only return for the error conditions.

lijiarui · 2017-02-28T10:29:10Z

You are right, it's really a bad design

coveralls · 2017-02-28T11:10:57Z

Changes Unknown when pulling 56be013 on lijiarui:273 into ** on wechaty:master**.

huan

I don't know what are you thinking. It seems you had lost your mind.

huan · 2017-02-28T12:50:08Z

src/room.ts

@@ -130,6 +130,9 @@ export class Room extends EventEmitter implements Sayable {
      const data = await contactGetter(this.id)
      log.silly('Room', `contactGetter(${this.id}) resolved`)
      this.rawObj = data
+      if (!this.rawObj.MemberList) {
+        throw new Error('no this.obj.memberList, set after contactGetter')


As I said before: should NOT throw when !this.rawObj.MemberList.

Because this is OK.

huan · 2017-02-28T12:50:57Z

src/room.ts

@@ -130,6 +130,9 @@ export class Room extends EventEmitter implements Sayable {
      const data = await contactGetter(this.id)
      log.silly('Room', `contactGetter(${this.id}) resolved`)
      this.rawObj = data
+      if (!this.rawObj.MemberList) {
+        throw new Error('no this.obj.memberList, set after contactGetter')
+      }
      await this.readyAllMembers(this.rawObj.MemberList)


await this.readyAllMembers(this.rawObj.MemberList || [])

huan · 2017-02-28T12:51:33Z

src/room.ts

@@ -203,6 +206,9 @@ export class Room extends EventEmitter implements Sayable {
      log.warn('Room', 'parse() on a empty rawObj?')
      return null
    }
+    if (!rawObj.MemberList) {
+      rawObj.MemberList = []


Delete them.

As I mentioned before, you should NOT change any value of rawObj.

coveralls · 2017-02-28T14:42:51Z

Changes Unknown when pulling 81c2400 on lijiarui:273 into ** on wechaty:master**.

huan

After this, I believe the code change can be reduced to no more than 5 lines.

huan · 2017-03-01T00:23:37Z

src/room.ts

@@ -130,7 +130,7 @@ export class Room extends EventEmitter implements Sayable {
      const data = await contactGetter(this.id)
      log.silly('Room', `contactGetter(${this.id}) resolved`)
      this.rawObj = data
-      await this.readyAllMembers(this.rawObj.MemberList)
+      await this.readyAllMembers(this.rawObj.MemberList || [])


Do not do it here.

Do it inside readyAllMembers()

huan · 2017-03-01T00:24:04Z

src/room.ts

-    const aliasMap   = this.parseMap(rawObj.MemberList, 'alias')
+    const memberList = this.parseMemberList(rawObj.MemberList || [])
+    const nameMap    = this.parseMap(rawObj.MemberList || [], 'name')
+    const aliasMap   = this.parseMap(rawObj.MemberList || [], 'alias')


The above three: Do not do it here.

Do it inside parse*() function.

huan · 2017-03-01T00:26:22Z

src/room.ts

-      nameMap,
-      aliasMap,
+      memberList: memberList,
+      nameMap:  nameMap,


Should keep using memberList instead of memberList: memberList.

huan

Better now.

load all memberList

72a3c1c

huan requested changes Feb 22, 2017

View reviewed changes

load memberlist

c1ea433

lijiarui closed this Feb 22, 2017

lijiarui reopened this Feb 23, 2017

lijiarui added 2 commits February 28, 2017 11:03

Merge branch 'master' of https://github.com/wechaty/wechaty into 273

2798eff

allow memberList absence

2f11cc1

lijiarui mentioned this pull request Feb 28, 2017

Room.alias() should return null if we have not set the alias in the room #283

Closed

huan requested changes Feb 28, 2017

View reviewed changes

modify as request

c7e7cf3

huan reviewed Feb 28, 2017

View reviewed changes

resign memberlist

56be013

huan requested changes Feb 28, 2017

View reviewed changes

not change rawObj

81c2400

huan requested changes Mar 1, 2017

View reviewed changes

code clean

53305da

huan approved these changes Mar 1, 2017

View reviewed changes

huan merged commit 52cde27 into wechaty:master Mar 1, 2017

lijiarui deleted the 273 branch March 1, 2017 11:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

load all memberList #275

load all memberList #275

lijiarui commented Feb 22, 2017

huan left a comment

lijiarui commented Feb 22, 2017

huan commented Feb 22, 2017

huan commented Feb 22, 2017

huan Feb 28, 2017

huan Feb 28, 2017

huan Feb 28, 2017

lijiarui Feb 28, 2017

huan Feb 28, 2017

huan Feb 28, 2017

coveralls commented Feb 28, 2017 •

edited

Loading

coveralls commented Feb 28, 2017 •

edited

Loading

huan left a comment

huan Feb 28, 2017

huan Feb 28, 2017

huan Feb 28, 2017

huan Feb 28, 2017

lijiarui commented Feb 28, 2017

coveralls commented Feb 28, 2017 •

edited

Loading

huan left a comment

huan Feb 28, 2017

huan Feb 28, 2017

huan Feb 28, 2017

coveralls commented Feb 28, 2017 •

edited

Loading

huan left a comment

huan Mar 1, 2017

huan Mar 1, 2017

huan Mar 1, 2017

huan left a comment

load all memberList #275

load all memberList #275

Conversation

lijiarui commented Feb 22, 2017

huan left a comment

Choose a reason for hiding this comment

lijiarui commented Feb 22, 2017

huan commented Feb 22, 2017

huan commented Feb 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 28, 2017 • edited Loading

coveralls commented Feb 28, 2017 • edited Loading

huan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lijiarui commented Feb 28, 2017

coveralls commented Feb 28, 2017 • edited Loading

huan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 28, 2017 • edited Loading

huan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huan left a comment

Choose a reason for hiding this comment

coveralls commented Feb 28, 2017 •

edited

Loading

coveralls commented Feb 28, 2017 •

edited

Loading

coveralls commented Feb 28, 2017 •

edited

Loading

coveralls commented Feb 28, 2017 •

edited

Loading