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

room.memberAll() & change room.member() query to 3 types #364

Merged
merged 8 commits into from Apr 4, 2017

Conversation

Projects
None yet
6 participants
@lijiarui
Member

lijiarui commented Mar 27, 2017

#334

  • add function room.memberAll():Contact[] correspond to room.member(): Contact
  • change room.member() query to 3 types:
    • name
    • roomAlias
    • contactAlias
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 27, 2017

Coverage Status

Coverage increased (+0.5%) to 56.932% when pulling d168f70 on lijiarui:functionMember into d6a0e86 on Chatie:master.

coveralls commented Mar 27, 2017

Coverage Status

Coverage increased (+0.5%) to 56.932% when pulling d168f70 on lijiarui:functionMember into d6a0e86 on Chatie:master.

1 similar comment
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 27, 2017

Coverage Status

Coverage increased (+0.5%) to 56.932% when pulling d168f70 on lijiarui:functionMember into d6a0e86 on Chatie:master.

coveralls commented Mar 27, 2017

Coverage Status

Coverage increased (+0.5%) to 56.932% when pulling d168f70 on lijiarui:functionMember into d6a0e86 on Chatie:master.

@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Mar 27, 2017

Member

This PR has significant serial problems that described as the following:

  1. Please only resolve one problem in one PR. (member for this one. mention should in another one)
  2. Please do not close an on-going PR for no reason. (Change the PR instead of close it)
  3. Please pull the latest code from the master branch before you send a PR. (this PR is over writing my last commits, which is terrible for the code base)
  4. Please refer the ISSUE that your PR is resolving. (Create a new issue if there's no such issue for your PR)
Member

zixia commented Mar 27, 2017

This PR has significant serial problems that described as the following:

  1. Please only resolve one problem in one PR. (member for this one. mention should in another one)
  2. Please do not close an on-going PR for no reason. (Change the PR instead of close it)
  3. Please pull the latest code from the master branch before you send a PR. (this PR is over writing my last commits, which is terrible for the code base)
  4. Please refer the ISSUE that your PR is resolving. (Create a new issue if there's no such issue for your PR)
@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Mar 27, 2017

Member
  1. I really don't want to resolve mention problem, but if I don't do this, when I run npm run dist, it will get the following error and cannot run the unit test.
src/message.ts(442,35): error TS2345: Argument of type '{ alias: string; }' is not assignable to parameter of type 'string'.

I have recovered message.ts this time and it cannot pass the test...

  1. Sorry for close #363 . Because I forgot to pull the latest code from the master branch, so I send this new PR which has pulled the latest code from the master branch before.

  2. I'm not sure why this PR overwriting your last commit, I feel sorry about this, but I guess maybe #363 do this by mistake.

  3. I referred Issue #334, maybe this issue is not that clear, I create an issue #365

Sorry for the mistake...

Member

lijiarui commented Mar 27, 2017

  1. I really don't want to resolve mention problem, but if I don't do this, when I run npm run dist, it will get the following error and cannot run the unit test.
src/message.ts(442,35): error TS2345: Argument of type '{ alias: string; }' is not assignable to parameter of type 'string'.

I have recovered message.ts this time and it cannot pass the test...

  1. Sorry for close #363 . Because I forgot to pull the latest code from the master branch, so I send this new PR which has pulled the latest code from the master branch before.

  2. I'm not sure why this PR overwriting your last commit, I feel sorry about this, but I guess maybe #363 do this by mistake.

  3. I referred Issue #334, maybe this issue is not that clear, I create an issue #365

Sorry for the mistake...

@lijiarui lijiarui referenced this pull request Mar 28, 2017

Merged

add room-leave event #370

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 29, 2017

Coverage Status

Coverage increased (+0.5%) to 56.998% when pulling d009af6 on lijiarui:functionMember into d6a0e86 on Chatie:master.

coveralls commented Mar 29, 2017

Coverage Status

Coverage increased (+0.5%) to 56.998% when pulling d009af6 on lijiarui:functionMember into d6a0e86 on Chatie:master.

@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Mar 29, 2017

Member

Could you merge this PR this time?

Member

lijiarui commented Mar 29, 2017

Could you merge this PR this time?

@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Mar 29, 2017

Member

No, I can't.

This PR has significant problems that described as the following: (you still have not resolve those problems)

  1. Please only resolve one problem in one PR. (member for this one. mention should in another one)
  2. Please pull the latest code from the master branch before you send a PR. (this PR is over writing my last commits, which is terrible for the code base)
  3. Please refer the ISSUE that your PR is resolving. (Create a new issue if there's no such issue for your PR)
Member

zixia commented Mar 29, 2017

No, I can't.

This PR has significant problems that described as the following: (you still have not resolve those problems)

  1. Please only resolve one problem in one PR. (member for this one. mention should in another one)
  2. Please pull the latest code from the master branch before you send a PR. (this PR is over writing my last commits, which is terrible for the code base)
  3. Please refer the ISSUE that your PR is resolving. (Create a new issue if there's no such issue for your PR)

lijiarui added some commits Mar 29, 2017

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 29, 2017

Coverage Status

Coverage increased (+0.5%) to 56.998% when pulling b6bd373 on lijiarui:functionMember into 3c38b6a on Chatie:master.

coveralls commented Mar 29, 2017

Coverage Status

Coverage increased (+0.5%) to 56.998% when pulling b6bd373 on lijiarui:functionMember into 3c38b6a on Chatie:master.

@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Mar 29, 2017

Member

Sorry, about your 3 questions:

  1. I have recover message.spec.ts and just change message.ts to fix member() bug.
  2. just have merger master branch.
  3. I have created a new issue #365.
Member

lijiarui commented Mar 29, 2017

Sorry, about your 3 questions:

  1. I have recover message.spec.ts and just change message.ts to fix member() bug.
  2. just have merger master branch.
  3. I have created a new issue #365.
@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Mar 29, 2017

Member

No, you didn't resolve all my questions.

At least you have not merged the master branch.
See: https://github.com/Chatie/wechaty/pull/364/files#diff-a260093009080ad4951308e3d5ea54fdL130

You are really deliver a very terrible result and waste us lots of time.

Member

zixia commented Mar 29, 2017

No, you didn't resolve all my questions.

At least you have not merged the master branch.
See: https://github.com/Chatie/wechaty/pull/364/files#diff-a260093009080ad4951308e3d5ea54fdL130

You are really deliver a very terrible result and waste us lots of time.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 29, 2017

Coverage Status

Coverage decreased (-0.05%) to 56.407% when pulling b6bd373 on lijiarui:functionMember into 3c38b6a on Chatie:master.

coveralls commented Mar 29, 2017

Coverage Status

Coverage decreased (-0.05%) to 56.407% when pulling b6bd373 on lijiarui:functionMember into 3c38b6a on Chatie:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 29, 2017

Coverage Status

Coverage increased (+0.2%) to 56.668% when pulling 70c5581 on lijiarui:functionMember into 3c38b6a on Chatie:master.

coveralls commented Mar 29, 2017

Coverage Status

Coverage increased (+0.2%) to 56.668% when pulling 70c5581 on lijiarui:functionMember into 3c38b6a on Chatie:master.

1 similar comment
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 29, 2017

Coverage Status

Coverage increased (+0.2%) to 56.668% when pulling 70c5581 on lijiarui:functionMember into 3c38b6a on Chatie:master.

coveralls commented Mar 29, 2017

Coverage Status

Coverage increased (+0.2%) to 56.668% when pulling 70c5581 on lijiarui:functionMember into 3c38b6a on Chatie:master.

@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Mar 30, 2017

Member

Sorry, how about this time?

Member

lijiarui commented Mar 30, 2017

Sorry, how about this time?

@zixia

You need to know more about SemVer and fix the breaking changes which you have made in this PR.

Show outdated Hide outdated src/room.ts Outdated
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 31, 2017

Coverage Status

Coverage decreased (-1.7%) to 54.731% when pulling f0b2fcf on lijiarui:functionMember into 3c38b6a on Chatie:master.

coveralls commented Mar 31, 2017

Coverage Status

Coverage decreased (-1.7%) to 54.731% when pulling f0b2fcf on lijiarui:functionMember into 3c38b6a on Chatie:master.

@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Mar 31, 2017

Member

@zixia Could it be merged this time?

Member

lijiarui commented Mar 31, 2017

@zixia Could it be merged this time?

@zixia

Please read my review and try to understand it carefully.

Thanks.

Show outdated Hide outdated src/room.ts Outdated

@zixia zixia requested review from dcsan, xinbenlv, Gcaufy and imWildCat Mar 31, 2017

@zixia zixia requested review from Samurais and mukaiu Mar 31, 2017

@imWildCat

Agree. It is necessary to be explicit.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 31, 2017

Coverage Status

Coverage decreased (-1.7%) to 54.743% when pulling 0946181 on lijiarui:functionMember into 3c38b6a on Chatie:master.

coveralls commented Mar 31, 2017

Coverage Status

Coverage decreased (-1.7%) to 54.743% when pulling 0946181 on lijiarui:functionMember into 3c38b6a on Chatie:master.

@zixia

zixia approved these changes Mar 31, 2017 edited

Thanks, the PR looks good for me now.

In order to make sure this PR is Okey for the community, please invite other reviewers to approve your PR, and get at least 3 approving, which means you need to get 1 more. (thanks @imWildCat for the fast response!)

After got 3 approving, I'll be able to merge this PR.

@dcsan

This comment has been minimized.

Show comment
Hide comment
@dcsan

dcsan Apr 1, 2017

@lijiarui I can try to help review, but can you explain a little more what I should check?
I'm always confused about what convention we are using for naming people. Is this related to cleaning up the names so that you can get Dashbot integration working?


add function room.memberAll():Contact[] correspond to room.member(): Contact
change room.member() query to 3 types:

  • name
  • roomAlias
  • contactAlias

dcsan commented Apr 1, 2017

@lijiarui I can try to help review, but can you explain a little more what I should check?
I'm always confused about what convention we are using for naming people. Is this related to cleaning up the names so that you can get Dashbot integration working?


add function room.memberAll():Contact[] correspond to room.member(): Contact
change room.member() query to 3 types:

  • name
  • roomAlias
  • contactAlias
@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Apr 1, 2017

Member

@dcsan Thanks
I'm trying to clean up the names, but not for Dashbot, because when I find room.member() cannot work well because of the complex names. see more in #365. About the name discussion, see more in #217.

Because sometimes 2 people may get the same name in one room, so I add memeberAll() get a contact list. And also, 2 people with the same name in one room seldom occur, so I keep function member() to get the first contact from the contactList. This is similar to function Contact.find() and function Contact.findAll()

Member

lijiarui commented Apr 1, 2017

@dcsan Thanks
I'm trying to clean up the names, but not for Dashbot, because when I find room.member() cannot work well because of the complex names. see more in #365. About the name discussion, see more in #217.

Because sometimes 2 people may get the same name in one room, so I add memeberAll() get a contact list. And also, 2 people with the same name in one room seldom occur, so I keep function member() to get the first contact from the contactList. This is similar to function Contact.find() and function Contact.findAll()

@mukaiu

mukaiu approved these changes Apr 4, 2017

Perfect,Can be more accurate positioning room members.

@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Apr 4, 2017

Member

@zixia Could you help to merge? Thanks

Member

lijiarui commented Apr 4, 2017

@zixia Could you help to merge? Thanks

@zixia zixia merged commit 859c226 into Chatie:master Apr 4, 2017

4 of 6 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
coverage/coveralls Coverage decreased (-1.7%) to 54.743%
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
security/snyk No new vulnerabilities
Details
@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Apr 4, 2017

Member

Great, thanks all.

Member

zixia commented Apr 4, 2017

Great, thanks all.

@lijiarui lijiarui deleted the lijiarui:functionMember branch Sep 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment