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

support brand checking of contact #404

Merged
merged 14 commits into from Apr 25, 2017

Conversation

Projects
None yet
5 participants
@JasLin
Contributor

JasLin commented Apr 12, 2017

@see https://github.com/Urinx/WeixinBot/blob/master/README.md

公众号/服务号	以@开头,但其VerifyFlag & 8 != 0 

VerifyFlag: 
         一般公众号/服务号:8 
         微信自家的服务号:24 
         微信官方账号微信团队:56

JasLin and others added some commits Mar 24, 2017

Merge pull request #5 from Chatie/master
upgrade to  latest code
jas@mac
1. in wxwebapp,first item in memberList is a random user from the roo…
…m, 2. some times we can got owneruin or ChatRoomOwner . as those reason, first member from memberlist should not be here any more
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 12, 2017

Coverage Status

Coverage decreased (-0.03%) to 54.759% when pulling 0e60d4e on JasLin:master into 7fcb128 on Chatie:master.

coveralls commented Apr 12, 2017

Coverage Status

Coverage decreased (-0.03%) to 54.759% when pulling 0e60d4e on JasLin:master into 7fcb128 on Chatie:master.

@zixia

zixia approved these changes Apr 12, 2017

Good work bro, It seems like a really tricky magic number inside.

LGTM and please get at least 3 approvements from our contributors before merge for enhancing our community communication.

Thanks!

Show outdated Hide outdated tslint.json Outdated
Show outdated Hide outdated src/contact.ts Outdated
Show outdated Hide outdated src/contact.ts Outdated
Show outdated Hide outdated example/contact-bot.ts Outdated
@JasLin

This comment has been minimized.

Show comment
Hide comment
@JasLin

JasLin Apr 12, 2017

Contributor

@lijiarui @Gcaufy @xinbenlv @dcsan @imWildCat @mukaiu @Samurais
hi guys , anybody have time to review this feature?

Contributor

JasLin commented Apr 12, 2017

@lijiarui @Gcaufy @xinbenlv @dcsan @imWildCat @mukaiu @Samurais
hi guys , anybody have time to review this feature?

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 12, 2017

Coverage Status

Coverage decreased (-0.02%) to 54.775% when pulling 33b7e4a on JasLin:master into 7fcb128 on Chatie:master.

coveralls commented Apr 12, 2017

Coverage Status

Coverage decreased (-0.02%) to 54.775% when pulling 33b7e4a on JasLin:master into 7fcb128 on Chatie:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 13, 2017

Coverage Status

Coverage decreased (-0.02%) to 54.775% when pulling 0b570d8 on JasLin:master into b225722 on Chatie:master.

coveralls commented Apr 13, 2017

Coverage Status

Coverage decreased (-0.02%) to 54.775% when pulling 0b570d8 on JasLin:master into b225722 on Chatie:master.

1 similar comment
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 13, 2017

Coverage Status

Coverage decreased (-0.02%) to 54.775% when pulling 0b570d8 on JasLin:master into b225722 on Chatie:master.

coveralls commented Apr 13, 2017

Coverage Status

Coverage decreased (-0.02%) to 54.775% when pulling 0b570d8 on JasLin:master into b225722 on Chatie:master.

@lijiarui

Cool!

Btw, could you help to review the add mention and room leave event feature for me ? Thanks!

@JasLin

This comment has been minimized.

Show comment
Hide comment
@JasLin

JasLin Apr 13, 2017

Contributor

@lijiarui thanks , i am trying to review them

Contributor

JasLin commented Apr 13, 2017

@lijiarui thanks , i am trying to review them

@JasLin JasLin referenced this pull request Apr 14, 2017

Merged

Add mention #362

@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Apr 14, 2017

Member

@dcsan I guess maybe you need this feature most, could you help to review this please? Thanks

Member

lijiarui commented Apr 14, 2017

@dcsan I guess maybe you need this feature most, could you help to review this please? Thanks

@dcsan

This comment has been minimized.

Show comment
Hide comment
@dcsan

dcsan Apr 16, 2017

@lijiarui I'm not sure what this feature does?

    • Check if it's a brand contact

what is a brand in this context? It usually means a product brand. Maybe you mean brand new ?
Sorry but I'm unclear.

dcsan commented Apr 16, 2017

@lijiarui I'm not sure what this feature does?

    • Check if it's a brand contact

what is a brand in this context? It usually means a product brand. Maybe you mean brand new ?
Sorry but I'm unclear.

@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Apr 16, 2017

Member

@dcsan brand here means wechat oa account

Generally ,wechaty cannot identify the contact is a real contact or a wechat oa account, but this pr can fix this problem.

@JasLin
I suggest to change brand to wechat oa here.

Member

lijiarui commented Apr 16, 2017

@dcsan brand here means wechat oa account

Generally ,wechaty cannot identify the contact is a real contact or a wechat oa account, but this pr can fix this problem.

@JasLin
I suggest to change brand to wechat oa here.

@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Apr 16, 2017

Member

Tencent has this title at https://admin.wechat.com/:
Wechat Official Account Admin Platform

  1. I suggest we might consider a rename, like from brand() to official()?

  2. Another thing is: should we include the 24 and 56 type in our brand() / official()(currently we only check for 8)? I think we might include them because then we can use !contact.official() to identify a personal user account.

VerifyFlag:
一般公众号/服务号:8
微信自家的服务号:24
微信官方账号微信团队:56

Member

zixia commented Apr 16, 2017

Tencent has this title at https://admin.wechat.com/:
Wechat Official Account Admin Platform

  1. I suggest we might consider a rename, like from brand() to official()?

  2. Another thing is: should we include the 24 and 56 type in our brand() / official()(currently we only check for 8)? I think we might include them because then we can use !contact.official() to identify a personal user account.

VerifyFlag:
一般公众号/服务号:8
微信自家的服务号:24
微信官方账号微信团队:56

@dcsan

This comment has been minimized.

Show comment
Hide comment
@dcsan

dcsan Apr 17, 2017

ok mingbai! so this is in the case where chatie bot is following an official account.
I'm not clear exactly what the use case would be for that - maybe testing other OAs?

so we have:

  • msg.room() - if it is a chat GROUP or a 1:1 (DM) chat
  • msg.brand() - if it's an OfficialAccount or ( a normal user OR group )

then service accounts, subscription accounts, personal OAs - not sure if the wechaty client gets any information about the type of brand/ official account, or if it matters.

msg.contact() - isn't really relevant for groups. maybe msg.sender() ?

then we could have a msg.sender().type() that returns

  • brand | user | group

as a string?

If we use enums btw I think we need to make these more easily exportable/importable to work with JS. Now we have async/await in node7 I've switched back to JS myself for most new projects.

dcsan commented Apr 17, 2017

ok mingbai! so this is in the case where chatie bot is following an official account.
I'm not clear exactly what the use case would be for that - maybe testing other OAs?

so we have:

  • msg.room() - if it is a chat GROUP or a 1:1 (DM) chat
  • msg.brand() - if it's an OfficialAccount or ( a normal user OR group )

then service accounts, subscription accounts, personal OAs - not sure if the wechaty client gets any information about the type of brand/ official account, or if it matters.

msg.contact() - isn't really relevant for groups. maybe msg.sender() ?

then we could have a msg.sender().type() that returns

  • brand | user | group

as a string?

If we use enums btw I think we need to make these more easily exportable/importable to work with JS. Now we have async/await in node7 I've switched back to JS myself for most new projects.

@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Apr 17, 2017

Member

@dcsan IMO stay with TypeScript is the bright decision, I love it to death now. :P

And for this VerifyFlag I think the most useful part is to identify if a contact is a personal account.

Now we can get all contact after login by Contact.findAll(), but the contactList is a mix of personal users and official accounts.

Add a contact.official() could sperate them easily by:

if (contact.official()) {
  // is Official Account
} else {
  // is Personal Account
}

Or

let contactList = await Contact.findAll()

let officialAccountList = contactList.filter(c => c.official())
let personalAccountList = contactList.filter(c => !c.officlal())
Member

zixia commented Apr 17, 2017

@dcsan IMO stay with TypeScript is the bright decision, I love it to death now. :P

And for this VerifyFlag I think the most useful part is to identify if a contact is a personal account.

Now we can get all contact after login by Contact.findAll(), but the contactList is a mix of personal users and official accounts.

Add a contact.official() could sperate them easily by:

if (contact.official()) {
  // is Official Account
} else {
  // is Personal Account
}

Or

let contactList = await Contact.findAll()

let officialAccountList = contactList.filter(c => c.official())
let personalAccountList = contactList.filter(c => !c.officlal())
@dcsan

This comment has been minimized.

Show comment
Hide comment
@dcsan

dcsan Apr 17, 2017

I think it's also useful to know if its a group(room) or an individual user, as some of the behavior is different, eg I don't think there is a contact for a room? (and not sure if there is a msg.room() or a topic() for an individual user. So you have to be careful with the logic or code can break, the types are not polymorphic.

I think i have stuff like:

topic = msg.room() ? msg.room().topic() : 'dm'

So when you have three major possible types, a boolean check method isn't as useful?


Personally I like Typescript a lot, but it is hard just getting new developers up and running, especially we have some team in Russia and other places. typescript, tslint, wechat api, docker... hours of setup to even get to helloworld.

dcsan commented Apr 17, 2017

I think it's also useful to know if its a group(room) or an individual user, as some of the behavior is different, eg I don't think there is a contact for a room? (and not sure if there is a msg.room() or a topic() for an individual user. So you have to be careful with the logic or code can break, the types are not polymorphic.

I think i have stuff like:

topic = msg.room() ? msg.room().topic() : 'dm'

So when you have three major possible types, a boolean check method isn't as useful?


Personally I like Typescript a lot, but it is hard just getting new developers up and running, especially we have some team in Russia and other places. typescript, tslint, wechat api, docker... hours of setup to even get to helloworld.

@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Apr 17, 2017

Member

Yes, I agree with you that TypeScript has a huge study curve. It seems things getting better after TypeScript v2.3 because they became treating unknown things to be any by default instead of throw errors, which will make JS developer easier to getting start.

And I do not fully understand you, as in the reply you said:

to know if its a group(room) or an individual user

To clarify, In Wechaty:

  1. room(group) will be an instance of Room and,
  2. contact(individual user) will be an instance of Contact. Room and Contact are totally two different Class(abstract);
  3. There is no contact for a room. Contact is for one, Room is for many;
  4. There is a message.room(): Room method to identify which room this message comes from;
  5. There is no contact.topic() because that's meaningless;

For your code:

topic = msg.room() ? msg.room().topic() : 'dm'

I'd like to rewrite it to:

const room = msg.room()
let topic: string
if (room) { 
  // this message is received in a room
  assert(room instanceof Room)
  topic = room.topic()
} else {
  // this message is received from a indivial user

  // WHY do you set room topic 
  // when you are receiving a direct message 
  // from an individual user?
  topic = 'dm' // ???
}
Member

zixia commented Apr 17, 2017

Yes, I agree with you that TypeScript has a huge study curve. It seems things getting better after TypeScript v2.3 because they became treating unknown things to be any by default instead of throw errors, which will make JS developer easier to getting start.

And I do not fully understand you, as in the reply you said:

to know if its a group(room) or an individual user

To clarify, In Wechaty:

  1. room(group) will be an instance of Room and,
  2. contact(individual user) will be an instance of Contact. Room and Contact are totally two different Class(abstract);
  3. There is no contact for a room. Contact is for one, Room is for many;
  4. There is a message.room(): Room method to identify which room this message comes from;
  5. There is no contact.topic() because that's meaningless;

For your code:

topic = msg.room() ? msg.room().topic() : 'dm'

I'd like to rewrite it to:

const room = msg.room()
let topic: string
if (room) { 
  // this message is received in a room
  assert(room instanceof Room)
  topic = room.topic()
} else {
  // this message is received from a indivial user

  // WHY do you set room topic 
  // when you are receiving a direct message 
  // from an individual user?
  topic = 'dm' // ???
}
@dcsan

This comment has been minimized.

Show comment
Hide comment
@dcsan

dcsan Apr 17, 2017

hi - thanks for the above.

    topic = msg.room() ? msg.room().topic() : 'dm'
    // WHY do you set room topic 
    // when you are receiving a direct message 
    // from an individual user?

yeah, it's a bit hacky agreed. it is so we can render all messages and handle group|dm display based on a single (cached) property rather than another chain of method calls.
But they do still have a different class hierarchy so it's not that correct.
Now we have to check two methods:

if ( msg.room() === null && msg.brand() === null ) then it's a DM

If we had msg.type() we could do:

switch (msg.type() ):
   case 'room':
      // responding in a group
   case 'dm':
     //..
   case 'oa':
    // brand / official account

Just a thought. This maybe still a hacky way to do this, as they are still different types of objects. Perhaps we'd be better with something like the Message < MediaMessage hierarchy.


re TypeScript, I think the language is easy.
it's all the setup of the tooling, linter, compiler (global tsc? compile output to where? .tsconfig settings?) especially for new apps. We are trying to break out more into microservices now, so we have a few smaller apps. this creates more setup overhead with TS.

dcsan commented Apr 17, 2017

hi - thanks for the above.

    topic = msg.room() ? msg.room().topic() : 'dm'
    // WHY do you set room topic 
    // when you are receiving a direct message 
    // from an individual user?

yeah, it's a bit hacky agreed. it is so we can render all messages and handle group|dm display based on a single (cached) property rather than another chain of method calls.
But they do still have a different class hierarchy so it's not that correct.
Now we have to check two methods:

if ( msg.room() === null && msg.brand() === null ) then it's a DM

If we had msg.type() we could do:

switch (msg.type() ):
   case 'room':
      // responding in a group
   case 'dm':
     //..
   case 'oa':
    // brand / official account

Just a thought. This maybe still a hacky way to do this, as they are still different types of objects. Perhaps we'd be better with something like the Message < MediaMessage hierarchy.


re TypeScript, I think the language is easy.
it's all the setup of the tooling, linter, compiler (global tsc? compile output to where? .tsconfig settings?) especially for new apps. We are trying to break out more into microservices now, so we have a few smaller apps. this creates more setup overhead with TS.

@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Apr 20, 2017

Member

@JasLin Hi bro,

I suggest:

  1. we rename brand() to official() and
  2. include all the VerifyFlag(8/24/56) and
  3. launch this PR for now.

Because we could use this method to identify whether the contact is an individual personal user, future design/changes could be discussed later.

How do you think?

Member

zixia commented Apr 20, 2017

@JasLin Hi bro,

I suggest:

  1. we rename brand() to official() and
  2. include all the VerifyFlag(8/24/56) and
  3. launch this PR for now.

Because we could use this method to identify whether the contact is an individual personal user, future design/changes could be discussed later.

How do you think?

@JasLin

This comment has been minimized.

Show comment
Hide comment
@JasLin

JasLin Apr 20, 2017

Contributor

@zixia sorry check this so later,brand() is come from wxwebapp source code ,but i love official() too.

I will try to finish this tomorrow. And, happy birthday bro!:)

Contributor

JasLin commented Apr 20, 2017

@zixia sorry check this so later,brand() is come from wxwebapp source code ,but i love official() too.

I will try to finish this tomorrow. And, happy birthday bro!:)

@JasLin

This comment has been minimized.

Show comment
Hide comment
@JasLin

JasLin Apr 21, 2017

Contributor

@zixia Hi bro, I just check the wxwebapp's source and the rawObj again . the following info is confirmed

  • we don't need to include all verifyFlag , use !!(VerifyFlag & 8) can identify all brand/offical account , check isBrandContact method from source
  • VerifyFlag should be define like this exactly:

VerifyFlag:
一般公众号/服务号:8 个人公众号: 8
微信自家的服务号:24 企业公众号: 24
微信官方账号微信团队:56

and two more thing :

  • should we add a method personal() {return !official()}to identify personal() account for convenient?
  • should we add special() to identify special account ? check the source isSpContact
Contributor

JasLin commented Apr 21, 2017

@zixia Hi bro, I just check the wxwebapp's source and the rawObj again . the following info is confirmed

  • we don't need to include all verifyFlag , use !!(VerifyFlag & 8) can identify all brand/offical account , check isBrandContact method from source
  • VerifyFlag should be define like this exactly:

VerifyFlag:
一般公众号/服务号:8 个人公众号: 8
微信自家的服务号:24 企业公众号: 24
微信官方账号微信团队:56

and two more thing :

  • should we add a method personal() {return !official()}to identify personal() account for convenient?
  • should we add special() to identify special account ? check the source isSpContact
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 21, 2017

Coverage Status

Coverage increased (+0.7%) to 54.869% when pulling a48e386 on JasLin:master into 93b1704 on Chatie:master.

coveralls commented Apr 21, 2017

Coverage Status

Coverage increased (+0.7%) to 54.869% when pulling a48e386 on JasLin:master into 93b1704 on Chatie:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 21, 2017

Coverage Status

Coverage increased (+0.7%) to 54.897% when pulling a48e386 on JasLin:master into 93b1704 on Chatie:master.

coveralls commented Apr 21, 2017

Coverage Status

Coverage increased (+0.7%) to 54.897% when pulling a48e386 on JasLin:master into 93b1704 on Chatie:master.

@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Apr 21, 2017

Member
VerifyFlag:
个人公众号: 8
企业公众号: 24
微信官方账号微信团队:56

I think we could translate them into bit position:

Type 10 2 Bit
Official Account 8 0000 1000 1 << 4
Business Verified 24 0001 1000 1 << 5
Tencent Verified 56 0011 1000 1 << 6

How about let's redesign official() like this:

enum OfficialType {
  OFFICIAL,
  BUSINESS,
  TENCENT,
}

official(type = OfficialType.OFFICIAL): boolean

By default, official() will return whether the contact is an official account(1<<4 bit set). If we want to know whether it's a business verified, we could call official(OfficialType.BUSINESS) or official(OfficialType.TENCENT), and we could extend to more types for the official account if needed in the future.

  • Abut the personal() method, I have no feeling about it. :P
  • About the isSpContact(), I have no idea about what it is. I also see other names isShieldContact() etc, I think we could talk about them in another issue?

Update: I saw you have already implemented the personal() & special(), it's ok for me, we can landing it first. :)

Member

zixia commented Apr 21, 2017

VerifyFlag:
个人公众号: 8
企业公众号: 24
微信官方账号微信团队:56

I think we could translate them into bit position:

Type 10 2 Bit
Official Account 8 0000 1000 1 << 4
Business Verified 24 0001 1000 1 << 5
Tencent Verified 56 0011 1000 1 << 6

How about let's redesign official() like this:

enum OfficialType {
  OFFICIAL,
  BUSINESS,
  TENCENT,
}

official(type = OfficialType.OFFICIAL): boolean

By default, official() will return whether the contact is an official account(1<<4 bit set). If we want to know whether it's a business verified, we could call official(OfficialType.BUSINESS) or official(OfficialType.TENCENT), and we could extend to more types for the official account if needed in the future.

  • Abut the personal() method, I have no feeling about it. :P
  • About the isSpContact(), I have no idea about what it is. I also see other names isShieldContact() etc, I think we could talk about them in another issue?

Update: I saw you have already implemented the personal() & special(), it's ok for me, we can landing it first. :)

@zixia

Please use https://github.com/Chatie/webwx-app-tracker to reference web wechat source.

Thanks!

Show outdated Hide outdated src/contact.ts Outdated
Show outdated Hide outdated src/contact.ts Outdated
Show outdated Hide outdated src/contact.ts Outdated
Show outdated Hide outdated src/contact.ts Outdated
@JasLin

This comment has been minimized.

Show comment
Hide comment
@JasLin

JasLin Apr 22, 2017

Contributor
  • actually , special contacts contain most of Tencent contacts , but i haven't checked that whether all the special|Tencent contact's verifyFlag is 56, because i can not find out a way to load all those contacts to my WeChat.
  • and , check the source code of wxwebapp, there is no verifyFlag defined as 24 and 56
    , so i guess that maybe Urinx's analysis about 24 and 56 is just a guess too:) , as u see in the document ,he think 微信自家的服务号 's flag is 24 , but in my bot, I found that most of 企业公众号's flag is 24 instead. since that , i am not sure about this exactly. but we can sure about that using !!(VerifyFlag & 8) can identify all brand/offical contacts

here are all flags defined in source code:

            CONTACTFLAG_CONTACT: 1,
            CONTACTFLAG_CHATCONTACT: 2,
            CONTACTFLAG_CHATROOMCONTACT: 4,
            CONTACTFLAG_BLACKLISTCONTACT: 8,
            CONTACTFLAG_DOMAINCONTACT: 16,
            CONTACTFLAG_HIDECONTACT: 32,
            CONTACTFLAG_FAVOURCONTACT: 64,
            CONTACTFLAG_3RDAPPCONTACT: 128,
            CONTACTFLAG_SNSBLACKLISTCONTACT: 256,
            CONTACTFLAG_NOTIFYCLOSECONTACT: 512,
            CONTACTFLAG_TOPCONTACT: 2048,
            ... ...
            MM_USERATTRVERIFYFALG_BIZ: 1
            , MM_USERATTRVERIFYFALG_FAMOUS: 2
            , MM_USERATTRVERIFYFALG_BIZ_BIG: 4
            , MM_USERATTRVERIFYFALG_BIZ_BRAND: 8
            , MM_USERATTRVERIFYFALG_BIZ_VERIFIED: 16

so I suggest we design official() at this time , after we find out what are 24, 56 exactly means , then we can change office() to official(type?): boolean.

  • personal() is just a convenient method for identifying individual personal contacts. this method maybe use more often than official()
Contributor

JasLin commented Apr 22, 2017

  • actually , special contacts contain most of Tencent contacts , but i haven't checked that whether all the special|Tencent contact's verifyFlag is 56, because i can not find out a way to load all those contacts to my WeChat.
  • and , check the source code of wxwebapp, there is no verifyFlag defined as 24 and 56
    , so i guess that maybe Urinx's analysis about 24 and 56 is just a guess too:) , as u see in the document ,he think 微信自家的服务号 's flag is 24 , but in my bot, I found that most of 企业公众号's flag is 24 instead. since that , i am not sure about this exactly. but we can sure about that using !!(VerifyFlag & 8) can identify all brand/offical contacts

here are all flags defined in source code:

            CONTACTFLAG_CONTACT: 1,
            CONTACTFLAG_CHATCONTACT: 2,
            CONTACTFLAG_CHATROOMCONTACT: 4,
            CONTACTFLAG_BLACKLISTCONTACT: 8,
            CONTACTFLAG_DOMAINCONTACT: 16,
            CONTACTFLAG_HIDECONTACT: 32,
            CONTACTFLAG_FAVOURCONTACT: 64,
            CONTACTFLAG_3RDAPPCONTACT: 128,
            CONTACTFLAG_SNSBLACKLISTCONTACT: 256,
            CONTACTFLAG_NOTIFYCLOSECONTACT: 512,
            CONTACTFLAG_TOPCONTACT: 2048,
            ... ...
            MM_USERATTRVERIFYFALG_BIZ: 1
            , MM_USERATTRVERIFYFALG_FAMOUS: 2
            , MM_USERATTRVERIFYFALG_BIZ_BIG: 4
            , MM_USERATTRVERIFYFALG_BIZ_BRAND: 8
            , MM_USERATTRVERIFYFALG_BIZ_VERIFIED: 16

so I suggest we design official() at this time , after we find out what are 24, 56 exactly means , then we can change office() to official(type?): boolean.

  • personal() is just a convenient method for identifying individual personal contacts. this method maybe use more often than official()
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 22, 2017

Coverage Status

Coverage increased (+0.7%) to 54.869% when pulling 68ebd91 on JasLin:master into 93b1704 on Chatie:master.

coveralls commented Apr 22, 2017

Coverage Status

Coverage increased (+0.7%) to 54.869% when pulling 68ebd91 on JasLin:master into 93b1704 on Chatie:master.

@zixia

zixia approved these changes Apr 22, 2017

Hi @JasLin ,

I hope you have a good weekend!

Your new code looks good for me, and I agree with you about the magic number and method name opinion.

Please get 3 approving from our @Chatie/contributor before I could merge it.

Thanks!

@JasLin

This comment has been minimized.

Show comment
Hide comment
@JasLin

JasLin Apr 23, 2017

Contributor

it seems we need more contributors ...

Contributor

JasLin commented Apr 23, 2017

it seems we need more contributors ...

@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Apr 23, 2017

Member

Yes, I definitely agree with you bro... @_@

Member

zixia commented Apr 23, 2017

Yes, I definitely agree with you bro... @_@

@zixia zixia merged commit 54d199f into Chatie:master Apr 25, 2017

5 of 6 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
ci/circleci Your tests passed on CircleCI!
Details
codacy/pr Good work! A positive pull request.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.7%) to 54.869%
Details
security/snyk No new vulnerabilities
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment