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

fix jsdoc flush issue #378 and minor fix on the doc examples #380

Merged
merged 18 commits into from Mar 31, 2017

Conversation

Projects
None yet
5 participants
@ax4
Member

ax4 commented Mar 31, 2017

#378 I have fixed this flush issue!

And also, I made 2 minor fix:

  • Fix 'set' => 'Set' of upper case.
  • Fix examples on contact.alias() function. User '.alias()' instead of '.remark // should be deprecated '

@zixia zixia requested a review from lijiarui Mar 31, 2017

@zixia

zixia requested changes Mar 31, 2017 edited

Thanks! (Please notice the outdated comments that I had just made, because your new push has new changes.:)

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

mukaiu and others added some commits Mar 31, 2017

Limit the size of the sending file (#376)
* Limit the size of the sending file

* limit vedio size

* add remark

* tslint

* Optimize videoMaxSize limit

* tslint
use .alias() instead of .remark()
contact.remark() should be deprecated
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 31, 2017

Coverage Status

Coverage remained the same at 54.561% when pulling f6ddcad on ax4:doc into d0df7a8 on Chatie:master.

coveralls commented Mar 31, 2017

Coverage Status

Coverage remained the same at 54.561% when pulling f6ddcad on ax4:doc into d0df7a8 on Chatie:master.

ax4 added some commits Mar 31, 2017

Show outdated Hide outdated src/contact.ts Outdated
Show outdated Hide outdated src/contact.ts Outdated
* if (ret) {
* console.log(`change ${contact.name()}'s alias successfully!`)
* } else {
* console.error('failed to change ${contact.name()}'s alias!')

This comment has been minimized.

@lijiarui

lijiarui Mar 31, 2017

Member

You should change ' to ` here, or it may cannot be parsed successfully.

console.error(`failed to change ${contact.name()}'s alias!`)
@lijiarui

lijiarui Mar 31, 2017

Member

You should change ' to ` here, or it may cannot be parsed successfully.

console.error(`failed to change ${contact.name()}'s alias!`)

This comment has been minimized.

@ax4

ax4 Mar 31, 2017

Member

Maybe we should unify the ' or ` issue.

I see a lot of ' , while having a lot of ` too.

On my laptop, they could both parsed successfully. with npm run doc and 'npm run lint' is okay.

@ax4

ax4 Mar 31, 2017

Member

Maybe we should unify the ' or ` issue.

I see a lot of ' , while having a lot of ` too.

On my laptop, they could both parsed successfully. with npm run doc and 'npm run lint' is okay.

This comment has been minimized.

@lijiarui

lijiarui Mar 31, 2017

Member

maybe ` is better

because sometimes there is ' in the log, it will cut off the log.

@lijiarui

lijiarui Mar 31, 2017

Member

maybe ` is better

because sometimes there is ' in the log, it will cut off the log.

This comment has been minimized.

@ax4

ax4 Mar 31, 2017

Member

Ok, I've setup issue #381 to fix the entire thing, tomorrow! resolved!

@ax4

ax4 Mar 31, 2017

Member

Ok, I've setup issue #381 to fix the entire thing, tomorrow! resolved!

@@ -449,7 +449,7 @@ export class Contact implements Sayable {
*
* @example
* ```ts
* const ret = await contact.remark('lijiarui')
* const ret = await contact.alias('lijiarui')
* if (ret) {
* console.log(`change ${contact.name()}'s alias successfully!`)
* } else {

This comment has been minimized.

@lijiarui

lijiarui Mar 31, 2017

Member

I was wrong here too... Please help me to fix

console.error(`failed to change ${contact.name()}'s alias!`
@lijiarui

lijiarui Mar 31, 2017

Member

I was wrong here too... Please help me to fix

console.error(`failed to change ${contact.name()}'s alias!`

This comment has been minimized.

@ax4

ax4 Mar 31, 2017

Member

Maybe we should unify the ' or ` issue.

I see a lot of ' , while having a lot of ` too.

On my laptop, they could both parsed successfully. with npm run doc and 'npm run lint' is okay.

@ax4

ax4 Mar 31, 2017

Member

Maybe we should unify the ' or ` issue.

I see a lot of ' , while having a lot of ` too.

On my laptop, they could both parsed successfully. with npm run doc and 'npm run lint' is okay.

This comment has been minimized.

@ax4

ax4 Mar 31, 2017

Member

resolved! Will fix the entire ' or ` issue at #381

@ax4

ax4 Mar 31, 2017

Member

resolved! Will fix the entire ' or ` issue at #381

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 31, 2017

Coverage Status

Coverage decreased (-0.05%) to 54.514% when pulling 462b2d9 on ax4:doc into d0df7a8 on Chatie:master.

coveralls commented Mar 31, 2017

Coverage Status

Coverage decreased (-0.05%) to 54.514% when pulling 462b2d9 on ax4:doc into d0df7a8 on Chatie:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 31, 2017

Coverage Status

Coverage decreased (-0.05%) to 54.514% when pulling cf934d7 on ax4:doc into d0df7a8 on Chatie:master.

coveralls commented Mar 31, 2017

Coverage Status

Coverage decreased (-0.05%) to 54.514% when pulling cf934d7 on ax4:doc into d0df7a8 on Chatie:master.

@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Mar 31, 2017

Member

@ax4
One last thing, src/puppet-web/puppet-web.ts is this change by you or others?
Maybe this file shouldn't be committed here.

Member

lijiarui commented Mar 31, 2017

@ax4
One last thing, src/puppet-web/puppet-web.ts is this change by you or others?
Maybe this file shouldn't be committed here.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 31, 2017

Coverage Status

Coverage decreased (-0.05%) to 54.514% when pulling cf934d7 on ax4:doc into d0df7a8 on Chatie:master.

coveralls commented Mar 31, 2017

Coverage Status

Coverage decreased (-0.05%) to 54.514% when pulling cf934d7 on ax4:doc into d0df7a8 on Chatie:master.

1 similar comment
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 31, 2017

Coverage Status

Coverage decreased (-0.05%) to 54.514% when pulling cf934d7 on ax4:doc into d0df7a8 on Chatie:master.

coveralls commented Mar 31, 2017

Coverage Status

Coverage decreased (-0.05%) to 54.514% when pulling cf934d7 on ax4:doc into d0df7a8 on Chatie:master.

@ax4

This comment has been minimized.

Show comment
Hide comment
@ax4

ax4 Mar 31, 2017

Member

@lijiarui Ok! I'm reverting the change on src/puppet-web/puppet-web.ts. I accidentally changed this file by rebase my branch from Chatie/main branch.

Working on it.

Update
Revert the change on 'src/puppet-web/puppet-web.ts' - done! resolved!

ALL Resolved!
@lijiarui @zixia Thanks!

Member

ax4 commented Mar 31, 2017

@lijiarui Ok! I'm reverting the change on src/puppet-web/puppet-web.ts. I accidentally changed this file by rebase my branch from Chatie/main branch.

Working on it.

Update
Revert the change on 'src/puppet-web/puppet-web.ts' - done! resolved!

ALL Resolved!
@lijiarui @zixia Thanks!

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 31, 2017

Coverage Status

Coverage remained the same at 54.561% when pulling ee6fc5e on ax4:doc into d0df7a8 on Chatie:master.

coveralls commented Mar 31, 2017

Coverage Status

Coverage remained the same at 54.561% when pulling ee6fc5e on ax4:doc into d0df7a8 on Chatie:master.

@zixia zixia merged commit ba18316 into Chatie:master Mar 31, 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 remained the same at 54.561%
Details
security/snyk No new vulnerabilities
Details
@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Mar 31, 2017

Member

@ax4 @lijiarui Thanks for the PR and the approving!

Member

zixia commented Mar 31, 2017

@ax4 @lijiarui Thanks for the PR and the approving!

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