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

Add JsDoc for Class Contact #321

Merged
merged 15 commits into from Mar 13, 2017

Conversation

Projects
None yet
2 participants
@lijiarui
Member

lijiarui commented Mar 12, 2017

#73
#252
use JsDoc to generate contact doc here.
Doc: Contact

@zixia

Good job. Please follow my reviews.

Show outdated Hide outdated docs/index.md Outdated
Show outdated Hide outdated src/contact.ts Outdated
Show outdated Hide outdated src/contact.ts Outdated
*
* `Contact` is `Sayable`

This comment has been minimized.

@zixia

zixia Mar 12, 2017

Member

Please add example to each method.

like:

/**
 * @example
 * ```ts
 *   code here
 * ```
 */
@zixia

zixia Mar 12, 2017

Member

Please add example to each method.

like:

/**
 * @example
 * ```ts
 *   code here
 * ```
 */

This comment has been minimized.

@lijiarui

lijiarui Mar 13, 2017

Member

I don't think we should add each method, some method like Contact.name(), the code already can explain itself as Gender you said.

I have added method Contact.find() in this commit.

But your suggestion make sense, except method find, I think I should add the following method:

  • avatar()
  • say()

What other functions do you think I should add too?

@lijiarui

lijiarui Mar 13, 2017

Member

I don't think we should add each method, some method like Contact.name(), the code already can explain itself as Gender you said.

I have added method Contact.find() in this commit.

But your suggestion make sense, except method find, I think I should add the following method:

  • avatar()
  • say()

What other functions do you think I should add too?

This comment has been minimized.

@zixia

zixia Mar 13, 2017

Member

I do not agree. The Document is different to the Code.

Examples can help users to understand easier how to use it rather than reading the API reference.

Now what you think it's obviously but for others are not, that's because you are too familiar with Wechaty.

I think to add examples for all API functions is necessary, but I'll leave this decision to you.

Please add examples to the APIs which you think is needed, leave it without example as you like.

@zixia

zixia Mar 13, 2017

Member

I do not agree. The Document is different to the Code.

Examples can help users to understand easier how to use it rather than reading the API reference.

Now what you think it's obviously but for others are not, that's because you are too familiar with Wechaty.

I think to add examples for all API functions is necessary, but I'll leave this decision to you.

Please add examples to the APIs which you think is needed, leave it without example as you like.

This comment has been minimized.

@lijiarui

lijiarui Mar 13, 2017

Member

You are right, maybe I'm too familiar with Wechaty, so I add examples for all API functions.
And I added all the methods examples

@lijiarui

lijiarui Mar 13, 2017

Member

You are right, maybe I'm too familiar with Wechaty, so I add examples for all API functions.
And I added all the methods examples

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
@zixia

You did not follow my last reviews.

Show outdated Hide outdated docs/index.md Outdated
Show outdated Hide outdated src/contact.ts Outdated
Show outdated Hide outdated src/contact.ts Outdated

lijiarui added some commits Mar 13, 2017

@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Mar 13, 2017

Member

@zixia
done all of your request.

  • @memberof
    removed all @memberOf
  • example
    You are right, maybe I'm too familiar with Wechaty, so I add examples for all API functions.
Member

lijiarui commented Mar 13, 2017

@zixia
done all of your request.

  • @memberof
    removed all @memberOf
  • example
    You are right, maybe I'm too familiar with Wechaty, so I add examples for all API functions.
@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Mar 13, 2017

Member

No, you are not done.

Please mark all my reviews as resolved after you confirmed it is resolved.

I noticed that there has some reviews you did not reply/react.

Member

zixia commented Mar 13, 2017

No, you are not done.

Please mark all my reviews as resolved after you confirmed it is resolved.

I noticed that there has some reviews you did not reply/react.

@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Mar 13, 2017

Member

How about this time?

Member

lijiarui commented Mar 13, 2017

How about this time?

@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Mar 13, 2017

Member

I don't know.

Please tell me you are done after you are done.

Member

zixia commented Mar 13, 2017

I don't know.

Please tell me you are done after you are done.

@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Mar 13, 2017

Member

done

Member

lijiarui commented Mar 13, 2017

done

@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Mar 13, 2017

Member

I'm sorry....

I thought I used wrong git command...
I should use git rm –cached instead of git rm to delete index instead of delete both index and the file.

But I thought your bot can generate the docs automatically.

Member

lijiarui commented Mar 13, 2017

I'm sorry....

I thought I used wrong git command...
I should use git rm –cached instead of git rm to delete index instead of delete both index and the file.

But I thought your bot can generate the docs automatically.

@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Mar 13, 2017

Member

No, you are not done.

  1. you leave one un-resolved review.
  2. you should not commit the file docs/index.md. this time you still commit it, with worse behaviour: deleted it. I need your PR only include one file modification instead of two.
Member

zixia commented Mar 13, 2017

No, you are not done.

  1. you leave one un-resolved review.
  2. you should not commit the file docs/index.md. this time you still commit it, with worse behaviour: deleted it. I need your PR only include one file modification instead of two.
@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Mar 13, 2017

Member

done

Member

lijiarui commented Mar 13, 2017

done

@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Mar 13, 2017

Member

Not yet.

Member

zixia commented Mar 13, 2017

Not yet.

@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Mar 13, 2017

Member

I'm not sure which haven't done....

1.you leave one un-resolved review.
2.you should not commit the file docs/index.md. this time you still commit it, with worse behaviour: deleted it. I need your PR only include one file modification instead of two.

  1. I have replyed your example review and added all methods examples in c367b99
  2. I have recovered file I deleted in 1e16c0b

So could you tell me more?
Thanks.

Member

lijiarui commented Mar 13, 2017

I'm not sure which haven't done....

1.you leave one un-resolved review.
2.you should not commit the file docs/index.md. this time you still commit it, with worse behaviour: deleted it. I need your PR only include one file modification instead of two.

  1. I have replyed your example review and added all methods examples in c367b99
  2. I have recovered file I deleted in 1e16c0b

So could you tell me more?
Thanks.

@zixia

Please fulfill all the return type for functions, and make sure the function body is returning the right type.

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
Show outdated Hide outdated src/contact.ts Outdated
Show outdated Hide outdated src/contact.ts Outdated
@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Mar 13, 2017

Member

Last time you leave this review unresolved:

image

Member

zixia commented Mar 13, 2017

Last time you leave this review unresolved:

image

@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Mar 13, 2017

Member

done fulfill all the return type for functions...
But I'm not sure whether I have resolved the example review...
Could you tell me?
Thanks

Member

lijiarui commented Mar 13, 2017

done fulfill all the return type for functions...
But I'm not sure whether I have resolved the example review...
Could you tell me?
Thanks

@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Mar 13, 2017

Member

You still leave one review unresolved.

But it's ok, I'll merge this PR now.

Member

zixia commented Mar 13, 2017

You still leave one review unresolved.

But it's ok, I'll merge this PR now.

@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Mar 13, 2017

Member

BTW: there's a bug that causing the unit test failed, it seems related to the PR from you recently.

Please have a look when you have time. Thanks.

Member

zixia commented Mar 13, 2017

BTW: there's a bug that causing the unit test failed, it seems related to the PR from you recently.

Please have a look when you have time. Thanks.

@zixia zixia merged commit b3a876d into Chatie:master Mar 13, 2017

2 of 5 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
ci/circleci CircleCI is running your tests
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
codacy/pr Good work! A positive pull request.
Details
security/snyk No new vulnerabilities
Details
@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Mar 13, 2017

Member

Could you tell me how to resolve the review?
Thanks

Member

lijiarui commented Mar 13, 2017

Could you tell me how to resolve the review?
Thanks

@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Mar 13, 2017

Member

I do not know because I can not see your account.

Member

zixia commented Mar 13, 2017

I do not know because I can not see your account.

@lijiarui lijiarui deleted the lijiarui:testDoc branch Mar 27, 2017

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