rename the nick/remark/display for contact/room #217 #234

lijiarui · 2017-02-06T05:56:03Z

No description provided.

coveralls · 2017-02-06T06:14:16Z

Coverage decreased (-0.2%) to 55.733% when pulling 39e1955 on lijiarui:217 into ee51219 on wechaty:master.

coveralls · 2017-02-06T06:14:16Z

Coverage decreased (-0.2%) to 55.733% when pulling 39e1955 on lijiarui:217 into ee51219 on wechaty:master.

coveralls · 2017-02-06T06:50:19Z

Coverage decreased (-0.002%) to 55.886% when pulling c0a6998 on lijiarui:217 into ee51219 on wechaty:master.

coveralls · 2017-02-06T06:50:19Z

Coverage decreased (-0.002%) to 55.886% when pulling c0a6998 on lijiarui:217 into ee51219 on wechaty:master.

huan

Please follow my change requests.

huan · 2017-02-06T09:49:34Z

src/contact.ts

@@ -55,8 +55,10 @@ export enum Gender {
 }

 export type ContactQueryFilter = {
-  name?: string | RegExp
+  name?:   string | RegExp
+  // remark should be deprecated
  remark?: string | RegExp


there's no reason to keep remark here anymore. remove it.

Then we should deprecate function Contact.find(remark: string) right now?

deprecate right now. but it should still functional.

ok, I guess I can use function Contact.find(query: ContactQueryFilter | {remark: string}) instead, is it right?

huan · 2017-02-06T09:51:21Z

src/contact.ts

@@ -232,6 +234,10 @@ export class Contact implements Sayable {
      query = { name: /.*/ }
    }

+    if (query[0] === 'remark') {
+      log.warn('Contact', 'findAll(remark:%s) DEPRECATED, use findAll(alias:%s) instead.')


add query[0] = 'alias' to the next line.

then you can only deal with alias in the following code, without remark anymore.

no need to add query[0] = 'alias', because I had deal the alias in the following code:

const keyMap = { name: 'NickName', // should be deprecated remark: 'RemarkName', alias: 'RemarkName', } ... if (filterValue instanceof RegExp) { filterFunction = `(function (c) { return ${filterValue.toString()}.test(c.${filterKey}) })` } else if (typeof filterValue === 'string') { filterValue = filterValue.replace(/'/g, '\\\'') filterFunction = `(function (c) { return c.${filterKey} === '${filterValue}' })` } else { throw new Error('unsupport name type') }

If you are sure to deprecate function Contact.find(remark:string) right now, I will delete this

no, you misunderstand.

you should remove remark in keyMap first, then you will have to follow my request.

huan · 2017-02-06T09:51:37Z

src/contact.ts

@@ -248,7 +254,9 @@ export class Contact implements Sayable {

    const keyMap = {
      name:   'NickName',
+      // should be deprecated
      remark: 'RemarkName',


remove remark

huan · 2017-02-06T09:56:01Z

src/contact.ts

                    return false // fail safe
                  })
  }

+  public remark(newRemark?: string|null): Promise<boolean> | string | null {


Add a // @deprecated comment above the function

huan · 2017-02-06T10:04:45Z

src/room.ts

-  displayMap: Map<string, string>
+  nameMap:      Map<string, string>
+  aliasMap:     Map<string, string>
+  roomAliasMap: Map<string, string>


Remove roomAliasMap here. Because aliasMap should be roomAliasMap

The room should only in charge of search alias in room. Search contact alias should use Contact.find() instead.

If we use Contact.find() instead, then when use function member() to get Contact would be asynchronous， which means it will use
public async member(queryArg: MemberQueryFilter | string): Promise<Contact | null>
but actually we want to use synchronous function.

because we use function member() in firer.ts

And, we must use alias(remark) in room, which I have said in #104 :

You said in room.ts

ISSUE #104 never use remark name because sys group message will never use that

But sys group message use remark.

For example, when the bot set remark for A, then A changed the topic, then sys message shows the remark, not roomAlias.

I have summarize in #173:

Display order:

@ Event

When someone @ a contact in a room, wechat recognise the name order as follows:
displayName > nickName

system message

When room event happens(join, leave, changetopic), system message recognise the name order as follows:
remark > nickName

on-screen Names

The contact name showed to the wechat user in the group
remark > displayName > nickName

So，I recommend we should use three kind of map:

nameMap
aliasMap
roomAliasMap

Ok, you are right.

Agreed.

huan · 2017-02-06T10:05:42Z

src/room.ts

 }

-type NameType = 'nick' | 'display' | 'remark'
+type NameType = 'name' | 'alias' | 'roomAlias'


no roomAlias. alias is for room only.

huan · 2017-02-06T10:05:54Z

src/room.ts

-  display?: string
+  name?:      string
+  alias?:     string
+  roomAlias?: string


no roomAlias.

coveralls · 2017-02-06T13:46:02Z

Coverage decreased (-0.02%) to 55.873% when pulling 1b88912 on lijiarui:217 into ee51219 on wechaty:master.

huan

Good, please follow my new reviews.

Also, please make sure the test coverage is not decreasing. last time your PR caused a coverage decrease:

Coverage decreased (-0.002%) to 55.886% when pulling c0a6998 on lijiarui:217 into ee51219

huan · 2017-02-06T13:38:08Z

src/contact.ts

-  public static async find(query: ContactQueryFilter): Promise<Contact> {
-    log.verbose('Contact', 'find(%s)', query.name)
+  public static async find(query: ContactQueryFilter | {remark: string | RegExp}): Promise<Contact> {
+    log.verbose('Contact', 'find(%s)', query)

    const contactList = await Contact.findAll(query)


should also log.warn() for remark in find() function.

I think no need to add log.warn() here, because find() use findAll(), and I had add log.warn() for findAll().

Do you agree?

huan · 2017-02-06T13:40:25Z

src/room.ts

  public nick(contact: Contact): string {
-    if (!this.obj || !this.obj.nickMap) {
+    log.warn('Room', 'nick(Contact) DEPRECATED, use name(Contact) instead.')


name(Contact) here should be alias(Contact)?

huan · 2017-02-06T13:42:45Z

src/room.ts

@@ -399,7 +406,7 @@ export class Room extends EventEmitter implements Sayable {

  public member(queryArg: MemberQueryFilter | string): Contact | null {
    if (typeof queryArg === 'string') {
-      return this.member({remark: queryArg}) || this.member({display: queryArg}) || this.member({nick: queryArg})
+      return this.member({alias: queryArg}) || this.member({roomAlias: queryArg}) || this.member({name: queryArg})


here alias & roomAlias is a bit confused.

consider to rename to contactAlias & roomAlias to declare them more clearly.

lijiarui · 2017-02-06T13:53:08Z

I guess Coverage decrease because I deleted const name2 = r.name(contact2) in room.spec.ts, because it is a wrong function caused by me in the first commit.

I'm sorry in my first commit I add a wrong function Room.name(contact), but after read your request, I changed function name() into alias().

coveralls · 2017-02-07T10:25:00Z

Coverage decreased (-0.05%) to 55.836% when pulling 04987d0 on lijiarui:217 into ee51219 on wechaty:master.

huan

There's part of code is confusing me a lot.

huan · 2017-02-07T13:51:42Z

src/contact.ts

@@ -227,9 +227,9 @@ export class Contact implements Sayable {
  public static async findAll(queryArg?: ContactQueryFilter | {remark: string | RegExp}): Promise<Contact[]> {
    let query: ContactQueryFilter
    if (queryArg) {
-      if (queryArg[0] === 'remark') {
+      if (Object.keys(queryArg)[0] === 'remark') {


change to: if (queryArg.remark) {

I tried it before, but it isn't compiled successfully:

public static async findAll(queryArg?: ContactQueryFilter | {remark: string | RegExp}): Promise<Contact[]> { let query: ContactQueryFilter if (queryArg) { if (queryArg.remark) { query = { alias: queryArg.remark} }

error throw:

src/contact.ts(230,20): error TS2339: Property 'remark' does not exist on type 'ContactQueryFilter | { remark: string | RegExp; }'. Property 'remark' does not exist on type 'ContactQueryFilter'. src/contact.ts(231,35): error TS2339: Property 'remark' does not exist on type 'ContactQueryFilter | { remark: string | RegExp; }'. Property 'remark' does not exist on type 'ContactQueryFilter'.

huan · 2017-02-07T13:52:30Z

src/contact.ts

        log.warn('Contact', 'Contact.findAll(remark:%s) DEPRECATED, use Contact.findAll(alias:%s) instead.')
-        query = { alias: queryArg[1]}
+        query = { alias: queryArg['remark']}


change to: query = { alias: queryArg.remark }

it will throw error

huan · 2017-02-07T13:53:24Z

src/room.ts

 }

-type NameType = 'name' | 'alias' | 'roomAlias'
+type NameType = 'nick' | 'alias'


nick ??? why nick, I think it should be name?

sorry, my fault..

huan · 2017-02-07T14:00:29Z

src/room.ts

    return this.alias(contact)
  }

  public alias(contact: Contact): string {
-    if (!this.obj || !this.obj.nameMap) {
+    if (!this.obj) {
      return ''


should return null be better?

huan · 2017-02-07T14:00:59Z

src/room.ts

      return ''
    }
-    return this.obj.roomAliasMap[contact.id] || this.obj.nameMap[contact.id]
+    return this.obj.aliasMap[contact.id]


if no aliasMap[contact.id], here'll return a undefined, which is not expected?

if no aliasMap[contact.id] here, it should return undefiend. Actually, I have deal alias in parse aliasMap as follows:

private parseMap(memberList: RoomRawMember[], parseContent: NameType): Map<string, string> { const mapList: Map<string, string> = new Map<string, string>() if (memberList && memberList.map) { memberList.forEach(member => { let tmpName: string let contact = Contact.load(member.UserName) switch (parseContent) { case 'name': tmpName = contact.alias() || contact.name() break case 'alias': tmpName = member.DisplayName || contact.name() break default: throw new Error('parseMap failed, member not found') }

if no DisplayName, this code tmpName = member.DisplayName || contact.name() will get contact.name()

So, no need to add this.obj.nameMap[contact.id] here, what do you think about it?

huan · 2017-02-07T14:16:05Z

src/contact.ts

-      query = queryArg
+      if (Object.keys(queryArg)[0] === 'remark') {
+        log.warn('Contact', 'Contact.findAll(remark:%s) DEPRECATED, use Contact.findAll(alias:%s) instead.')
+        query = { alias: queryArg['remark']}


queryArg.remark

huan · 2017-02-07T14:16:29Z

src/contact.ts

    let query: ContactQueryFilter
    if (queryArg) {
-      query = queryArg
+      if (Object.keys(queryArg)[0] === 'remark') {


if (queryArg.remark) {

huan · 2017-02-07T14:20:26Z

src/contact.ts

+    if (newRemark) {
+      return this.alias(newRemark)
+    } else {
+      return this.alias()


when newRemark = null, will run code here, which is not expected.

huan · 2017-02-07T14:24:04Z

src/room.ts

-    const remarkMap   = this.parseMap(rawObj.MemberList, 'remark')
-    const displayMap  = this.parseMap(rawObj.MemberList, 'display')
+    const memberList = this.parseMemberList(rawObj.MemberList)
+    const nameMap    = this.parseMap(rawObj.MemberList, 'nick')


why nameMap use arg name with nick here?

should be consistent.

huan · 2017-02-07T14:25:06Z

src/room.ts

@@ -238,21 +234,20 @@ export class Room extends EventEmitter implements Sayable {
        let contact = Contact.load(member.UserName)
        switch (parseContent) {
          case 'nick':


confusing names & logic here...

huan

Almost merage-able.

huan · 2017-02-08T02:54:12Z

src/room.ts

    }
-    return this.obj.displayMap[contact.id] || this.obj.nickMap[contact.id]
+    return this.obj.aliasMap[contact.id]


return this.obj.aliasMap[contact.id] || null

huan · 2017-02-08T02:56:11Z

src/contact.ts

   */
-  public static async findAll(queryArg?: ContactQueryFilter): Promise<Contact[]> {
+  public static async findAll(queryArg?: ContactQueryFilter | {remark: string | RegExp}): Promise<Contact[]> {


move remark: string | RegExp to ContactQueryFilter with a comment noticing DEPRECATED

then you could use .remark again

there's no reason to keep remark here anymore. remove it.

you just told me to remove remark from ContactQueryFilter

Ok, let me tell u one more:

Next time, if u still only know what instead of knowing why, and make stupid argue like this, then kick your ass 10 times by yourself.

rename the nick/remark/display for contact/room wechaty#217

39e1955

add test data

c0a6998

huan reviewed Feb 6, 2017

View reviewed changes

wechaty#217

1b88912

huan requested changes Feb 6, 2017

View reviewed changes

1

04987d0

huan requested changes Feb 7, 2017

View reviewed changes

2

da58f90

huan requested changes Feb 8, 2017

View reviewed changes

3

b0451d8

huan merged commit 03d3991 into wechaty:master Feb 8, 2017

lijiarui deleted the 217 branch February 8, 2017 03:20

rename the nick/remark/display for contact/room #217 #234

rename the nick/remark/display for contact/room #217 #234

Conversation

lijiarui commented Feb 6, 2017 • edited Loading

coveralls commented Feb 6, 2017

coveralls commented Feb 6, 2017

coveralls commented Feb 6, 2017

coveralls commented Feb 6, 2017

huan left a comment • edited Loading

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

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 Feb 6, 2017 • edited Loading

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 6, 2017 • edited Loading

huan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lijiarui Feb 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lijiarui commented Feb 6, 2017

coveralls commented Feb 7, 2017 • edited Loading

huan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lijiarui Feb 8, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lijiarui Feb 8, 2017 • edited Loading

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

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 6, 2017 •

edited

Loading

huan left a comment •

edited

Loading

lijiarui Feb 6, 2017 •

edited

Loading

coveralls commented Feb 6, 2017 •

edited

Loading

lijiarui Feb 6, 2017 •

edited

Loading

coveralls commented Feb 7, 2017 •

edited

Loading

lijiarui Feb 8, 2017 •

edited

Loading

lijiarui Feb 8, 2017 •

edited

Loading