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

#4 send image/video #337

Merged
merged 16 commits into from Mar 30, 2017

Conversation

Projects
None yet
6 participants
@mukaiu
Contributor

mukaiu commented Mar 19, 2017

Support Message Type of Image/Video for #4

Add
wechaty.uploadMedia(stream: NodeJS.ReadableStream, filename: string, toUserName: string)
wechaty.sendMedia(message: Message)

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 19, 2017

Coverage Status

Coverage decreased (-1.2%) to 54.596% when pulling 1bb2b9f on mukaiu:master into aab23c3 on Chatie:master.

coveralls commented Mar 19, 2017

Coverage Status

Coverage decreased (-1.2%) to 54.596% when pulling 1bb2b9f on mukaiu:master into aab23c3 on Chatie:master.

1 similar comment
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 19, 2017

Coverage Status

Coverage decreased (-1.2%) to 54.596% when pulling 1bb2b9f on mukaiu:master into aab23c3 on Chatie:master.

coveralls commented Mar 19, 2017

Coverage Status

Coverage decreased (-1.2%) to 54.596% when pulling 1bb2b9f on mukaiu:master into aab23c3 on Chatie:master.

@Samurais

This comment has been minimized.

Show comment
Hide comment
@Samurais

Samurais Mar 19, 2017

@zixia @lijiarui, @mukaiu has tested the functions. Please give some comments. Thanks.

Samurais commented Mar 19, 2017

@zixia @lijiarui, @mukaiu has tested the functions. Please give some comments. Thanks.

@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Mar 19, 2017

Member

@Samurais Great, I'll have a look into it.

@mukaiu Good job! 👍

Member

zixia commented Mar 19, 2017

@Samurais Great, I'll have a look into it.

@mukaiu Good job! 👍

@zixia

Hi @mukaiu , Great job, the PR looks great!

I had made some comments for you, mainly is talking about where is the right place to put those functions.

Please follow my reviews, let's move on and make it ready today. :)

P.S. I'm just thinking about the API of Wechaty of sending/replying files(PDF/Video/Photo/Emotions), it seems that we use stream for doing this is not the best choice.

For example, when user want to send/reply a file, the simpliest way is just use the full file path, because that's straight forward, and no need to specify the MIME type and filename anymore(which is required when we using stream).

So, maybe we could consider to use file path name in the API, at lease we support both types(file path name & stream).

Then we can design the API like this:

contact.say('/tmp/file.jpg')
room.say('/tmp/document.pdf')
message.reply('/tmp/haha.gif'))
Show outdated Hide outdated example/ding-img-bot.ts Outdated
Show outdated Hide outdated src/message.ts Outdated
Show outdated Hide outdated src/message.ts Outdated
Show outdated Hide outdated src/puppet-web/wechaty-bro.js Outdated
Show outdated Hide outdated src/puppet.ts Outdated
Show outdated Hide outdated src/puppet.ts Outdated
Show outdated Hide outdated src/wechaty.ts Outdated
Show outdated Hide outdated src/wechaty.ts Outdated
@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Mar 19, 2017

Member

Update:

I realized that contact.say('/tmp/file.jpg') will not work in sudden, because that's a string...

Do you have any suggestion for using the file path name directly for this scanerio?

Member

zixia commented Mar 19, 2017

Update:

I realized that contact.say('/tmp/file.jpg') will not work in sudden, because that's a string...

Do you have any suggestion for using the file path name directly for this scanerio?

@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Mar 19, 2017

Member

How about design a new type like

export type Content = {
    Type?: 'file' | 'pic' | 'voice' | ...
    Content: ''/tmp/file.jpg''
}

contact.say(para: Content | string)

Then, when we want to send voice or any other type of the content, just add Type

Even, we can add enum for Type

Member

lijiarui commented Mar 19, 2017

How about design a new type like

export type Content = {
    Type?: 'file' | 'pic' | 'voice' | ...
    Content: ''/tmp/file.jpg''
}

contact.say(para: Content | string)

Then, when we want to send voice or any other type of the content, just add Type

Even, we can add enum for Type

@dcsan

This comment has been minimized.

Show comment
Hide comment
@dcsan

dcsan Mar 19, 2017

I had a problem with this feature and created a ticket here:
#338

dcsan commented Mar 19, 2017

I had a problem with this feature and created a ticket here:
#338

@mukaiu

This comment has been minimized.

Show comment
Hide comment
@mukaiu

mukaiu Mar 19, 2017

Contributor

@dcsan
This is running ding-img-bot.ts?

Contributor

mukaiu commented Mar 19, 2017

@dcsan
This is running ding-img-bot.ts?

@dcsan

This comment has been minimized.

Show comment
Hide comment
@dcsan

dcsan Mar 19, 2017

@mukaiu yes

ts-node example/ding-img-bot.ts

(run as an npm script) please see #338 for more details.

dcsan commented Mar 19, 2017

@mukaiu yes

ts-node example/ding-img-bot.ts

(run as an npm script) please see #338 for more details.

@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Mar 21, 2017

Member

@lijiarui Yes, a new type is needed with this scenario.

What I'm thinking about is to create a new Class for doing this instead of an Interface. I.e. contact.say(new File('/tmp/a.pdf')) because this class could set the MIME information of the file automatically instead of by hand, which will be more convenience.

@mukaiu @JasLin I want to hear from you too because this API will be needed after this PR is merged.

Thanks!

Member

zixia commented Mar 21, 2017

@lijiarui Yes, a new type is needed with this scenario.

What I'm thinking about is to create a new Class for doing this instead of an Interface. I.e. contact.say(new File('/tmp/a.pdf')) because this class could set the MIME information of the file automatically instead of by hand, which will be more convenience.

@mukaiu @JasLin I want to hear from you too because this API will be needed after this PR is merged.

Thanks!

@mukaiu

This comment has been minimized.

Show comment
Hide comment
@mukaiu

mukaiu Mar 21, 2017

Contributor

I think,

contact.say(MediaMessage) 
Contributor

mukaiu commented Mar 21, 2017

I think,

contact.say(MediaMessage) 
@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Mar 21, 2017

Member

Yap, that's a better idea to solve this problem! :D

contact.say(new MediaMessage('/tmp/a.gif'))

👍 @mukaiu

Member

zixia commented Mar 21, 2017

Yap, that's a better idea to solve this problem! :D

contact.say(new MediaMessage('/tmp/a.gif'))

👍 @mukaiu

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 21, 2017

Coverage Status

Coverage decreased (-0.4%) to 55.328% when pulling e1755a0 on mukaiu:master into aab23c3 on Chatie:master.

coveralls commented Mar 21, 2017

Coverage Status

Coverage decreased (-0.4%) to 55.328% when pulling e1755a0 on mukaiu:master into aab23c3 on Chatie:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 21, 2017

Coverage Status

Coverage decreased (-0.4%) to 55.328% when pulling ecd2ff8 on mukaiu:master into 0426d48 on Chatie:master.

coveralls commented Mar 21, 2017

Coverage Status

Coverage decreased (-0.4%) to 55.328% when pulling ecd2ff8 on mukaiu:master into 0426d48 on Chatie:master.

#4 implement
contact.say(new MediaMessage('/tmp/file.jpg'))
message.say(new MediaMessage('/tmp/haha.gif'))
@mukaiu

This comment has been minimized.

Show comment
Hide comment
@mukaiu

mukaiu Mar 23, 2017

Contributor

@zixia
implement
contact.say(new MediaMessage('/tmp/file.jpg'))
message.say(new MediaMessage('/tmp/haha.gif'))

but the logic of room.say is uncertain.

Contributor

mukaiu commented Mar 23, 2017

@zixia
implement
contact.say(new MediaMessage('/tmp/file.jpg'))
message.say(new MediaMessage('/tmp/haha.gif'))

but the logic of room.say is uncertain.

@mukaiu

This comment has been minimized.

Show comment
Hide comment
@mukaiu

mukaiu Mar 23, 2017

Contributor

image
test error,help~~~

Contributor

mukaiu commented Mar 23, 2017

image
test error,help~~~

@dcsan

This comment has been minimized.

Show comment
Hide comment
@dcsan

dcsan Mar 25, 2017

Is there a docker image with this version of Wechaty? It would make testing much easier...

dcsan commented Mar 25, 2017

Is there a docker image with this version of Wechaty? It would make testing much easier...

@zixia

zixia requested changes Mar 25, 2017 edited

Hi @mukaiu ,

The new PR with enhanced media type upload is fantastic!

I just made some minor options in the comment; I think after finishing this we could be able to prepare the merging.

One more thing: about the circle dependency of Message and MediaMessage, I think we should consider solving this problem by merge message-media.ts to message.ts.

Show outdated Hide outdated src/contact.ts Outdated
Show outdated Hide outdated src/contact.ts Outdated
Show outdated Hide outdated src/message-media.ts Outdated
Show outdated Hide outdated src/message-media.ts Outdated
Show outdated Hide outdated src/message-media.ts Outdated
Show outdated Hide outdated src/message.ts Outdated
Show outdated Hide outdated src/message.ts Outdated
FileMd5: md5,
FromUserName: this.self().id,
ToUserName: toUserName,
UploadType: 2,

This comment has been minimized.

@zixia

zixia Mar 25, 2017

Member

Could you change this magic number to a enum value?

@zixia

zixia Mar 25, 2017

Member

Could you change this magic number to a enum value?

This comment has been minimized.

@mukaiu

mukaiu Mar 27, 2017

Contributor

UploadType optional value 1 or 2, use unknown

@mukaiu

mukaiu Mar 27, 2017

Contributor

UploadType optional value 1 or 2, use unknown

Show outdated Hide outdated src/puppet-web/puppet-web.ts Outdated
Show outdated Hide outdated src/puppet-web/puppet-web.ts Outdated
@mukaiu

This comment has been minimized.

Show comment
Hide comment
@mukaiu

mukaiu Mar 25, 2017

Contributor

@dcsan docker pull mukaiu/wechaty

Contributor

mukaiu commented Mar 25, 2017

@dcsan docker pull mukaiu/wechaty

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

mukaiu added some commits Mar 27, 2017

Merge branch 'master' into pr/4
# Conflicts:
#	package.json
Merge remote-tracking branch 'Chatie/master'
# Conflicts:
#	package.json
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 27, 2017

Coverage Status

Coverage decreased (-1.9%) to 54.59% when pulling 699f733 on mukaiu:master into d6a0e86 on Chatie:master.

coveralls commented Mar 27, 2017

Coverage Status

Coverage decreased (-1.9%) to 54.59% when pulling 699f733 on mukaiu:master into d6a0e86 on Chatie:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 27, 2017

Coverage Status

Coverage decreased (-1.9%) to 54.561% when pulling 699f733 on mukaiu:master into d6a0e86 on Chatie:master.

coveralls commented Mar 27, 2017

Coverage Status

Coverage decreased (-1.9%) to 54.561% when pulling 699f733 on mukaiu:master into d6a0e86 on Chatie:master.

@zixia zixia requested a review from Samurais Mar 28, 2017

@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Mar 28, 2017

Member

Hi @mukaiu,

Thank you so much for this Pull Request!

Now I feel it's good enough for supporting attachment messages in Wechaty, and I believe it will help many developers who want to use this functionality!

I have no more reviews for your PR, and I'll approve the changes.

However, We all know it's not perfect. I'm afraid there's some part that we missed to review. Could you talk with other Reviewers, and let some of them to Approve Changes?

Let's do merge the PR after at least other two contributors approved it. Cheers!

Member

zixia commented Mar 28, 2017

Hi @mukaiu,

Thank you so much for this Pull Request!

Now I feel it's good enough for supporting attachment messages in Wechaty, and I believe it will help many developers who want to use this functionality!

I have no more reviews for your PR, and I'll approve the changes.

However, We all know it's not perfect. I'm afraid there's some part that we missed to review. Could you talk with other Reviewers, and let some of them to Approve Changes?

Let's do merge the PR after at least other two contributors approved it. Cheers!

@zixia

zixia approved these changes Mar 28, 2017

@dcsan

This comment has been minimized.

Show comment
Hide comment
@dcsan

dcsan Mar 28, 2017

apologies for delay, I have been trying to check this out but haven't got a good docker usage development workflow, it takes too long to make a code change and test out, so I haven't been able to test it yet.

FWIW my flow is to build a container with mukai/wechaty fork as below
but then i have to rebuild the container, copy in my own code, run it...
since its a new container each time I also have to scan a QR code each dev loop.
this takes a few mins each code change.

is there a faster dev workflow you use?

I guess I could just run off a local build of wechaty, but I had problems with googlechrome and other deps, so aiming to use docker.

perhaps I can just exec bash on the docker container and work directly within it, but not sure how that works yet with the CMD [""] and ENTRYPOINT here
https://hub.docker.com/r/zixia/wechaty/~/dockerfile/

any other suggestions for fast workflow appreciated.

## using images fork
FROM mukaiu/wechaty
EXPOSE 8080
RUN mkdir -p /bot
WORKDIR /bot
COPY . /bot

CMD ["index.js"]

dcsan commented Mar 28, 2017

apologies for delay, I have been trying to check this out but haven't got a good docker usage development workflow, it takes too long to make a code change and test out, so I haven't been able to test it yet.

FWIW my flow is to build a container with mukai/wechaty fork as below
but then i have to rebuild the container, copy in my own code, run it...
since its a new container each time I also have to scan a QR code each dev loop.
this takes a few mins each code change.

is there a faster dev workflow you use?

I guess I could just run off a local build of wechaty, but I had problems with googlechrome and other deps, so aiming to use docker.

perhaps I can just exec bash on the docker container and work directly within it, but not sure how that works yet with the CMD [""] and ENTRYPOINT here
https://hub.docker.com/r/zixia/wechaty/~/dockerfile/

any other suggestions for fast workflow appreciated.

## using images fork
FROM mukaiu/wechaty
EXPOSE 8080
RUN mkdir -p /bot
WORKDIR /bot
COPY . /bot

CMD ["index.js"]
Show outdated Hide outdated src/puppet-web/puppet-web.ts Outdated
@mukaiu

This comment has been minimized.

Show comment
Hide comment
@mukaiu

mukaiu Mar 28, 2017

Contributor

@dcsan
Try using Docker Volume to mount the local directory to the docker bot directory.

docker run --name  bot -d -v LOCAL_DIR:/bot zixia/wechaty mybot.js

After modifying the code, restart container

docker restart bot

Use the following command to display the container log

docker logs -f bot
Contributor

mukaiu commented Mar 28, 2017

@dcsan
Try using Docker Volume to mount the local directory to the docker bot directory.

docker run --name  bot -d -v LOCAL_DIR:/bot zixia/wechaty mybot.js

After modifying the code, restart container

docker restart bot

Use the following command to display the container log

docker logs -f bot
@dcsan

This comment has been minimized.

Show comment
Hide comment
@dcsan

dcsan Mar 28, 2017

@mukaiu thanks i didn't know about docker volumes. perhaps if i combine that with hot reloading it will be much less painful to develop this way.

So i went to try and test this. in a wechaty checkout

  git checkout master
  git pull
  git fetch origin pull/337/head

but now it seems the ding-image bot doesnt exist, and there's no demo code in the ding-dong or media-file bot. Is there a usage example anywhere?

Since the API has changed, looking at your code above

message.say(new MediaMessage('/tmp/haha.gif'))

I modified ding-dong-bot.ts

import {
  MediaMessage,
} from '../'

// later
    if (/^(img)$/i.test(m.content()) ) {
      m.say(new MediaMessage('/image/wechaty-icon.png'))
      log.info('Bot', 'REPLY: dong')
    }

but gives TS compile error:

Argument of type 'MediaMessage' is not assignable to parameter of type 'string'.

I also looked in the /test directory for some examples but can't see anything:

grep -r Media test/*       
test/README.md:        # Subtest: Media download smoking test
test/README.md:    ok 2 - Media download smoking test # time=207.54ms
➜  wechaty git:(master) ✗ 

It would be great if there was some example code of how to use /test this functionality.

ts-node example/ding-dong-bot.ts 

/usr/local/lib/node_modules/ts-node/src/index.ts:319
          throw new TSError(formatDiagnostics(diagnosticList, cwd, ts, lineOffset))
                ^
TSError: ⨯ Unable to compile TypeScript
example/ding-dong-bot.ts (78,13): Argument of type 'MediaMessage' is not assignable to parameter of type 'string'. (2345)
    at getOutput (/usr/local/lib/node_modules/ts-node/src/index.ts:319:17)
    at /usr/local/lib/node_modules/ts-node/src/index.ts:350:18
    at Object.compile (/usr/local/lib/node_modules/ts-node/src/index.ts:509:17)
    at Module.m._compile (/usr/local/lib/node_modules/ts-node/src/index.ts:413:44)
    at Module._extensions..js (module.js:580:10)
    at Object.require.extensions.(anonymous function) [as .ts] (/usr/local/lib/node_modules/ts-node/src/index.ts:416:12)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Function.Module.runMain (module.js:605:10)

which is since Message.say signature is:

  public say(content: string, replyTo?: Contact|Contact[]): Promise<any> {

what is the interface to send a MediaMessage?

since export class MediaMessage extends Message {
I also tried sending a MediaMessage with an empty string.

    if (/^(img)$/i.test(m.content()) ) {
      let img = new MediaMessage('/image/wechaty-icon.png')
      img.say("")
      log.info('Bot', 'REPLY: dong')
    }

but gives error:

07:14:39 ERR Bot on(message) exception: SyntaxError: Unexpected token / in JSON at position 0

dcsan commented Mar 28, 2017

@mukaiu thanks i didn't know about docker volumes. perhaps if i combine that with hot reloading it will be much less painful to develop this way.

So i went to try and test this. in a wechaty checkout

  git checkout master
  git pull
  git fetch origin pull/337/head

but now it seems the ding-image bot doesnt exist, and there's no demo code in the ding-dong or media-file bot. Is there a usage example anywhere?

Since the API has changed, looking at your code above

message.say(new MediaMessage('/tmp/haha.gif'))

I modified ding-dong-bot.ts

import {
  MediaMessage,
} from '../'

// later
    if (/^(img)$/i.test(m.content()) ) {
      m.say(new MediaMessage('/image/wechaty-icon.png'))
      log.info('Bot', 'REPLY: dong')
    }

but gives TS compile error:

Argument of type 'MediaMessage' is not assignable to parameter of type 'string'.

I also looked in the /test directory for some examples but can't see anything:

grep -r Media test/*       
test/README.md:        # Subtest: Media download smoking test
test/README.md:    ok 2 - Media download smoking test # time=207.54ms
➜  wechaty git:(master) ✗ 

It would be great if there was some example code of how to use /test this functionality.

ts-node example/ding-dong-bot.ts 

/usr/local/lib/node_modules/ts-node/src/index.ts:319
          throw new TSError(formatDiagnostics(diagnosticList, cwd, ts, lineOffset))
                ^
TSError: ⨯ Unable to compile TypeScript
example/ding-dong-bot.ts (78,13): Argument of type 'MediaMessage' is not assignable to parameter of type 'string'. (2345)
    at getOutput (/usr/local/lib/node_modules/ts-node/src/index.ts:319:17)
    at /usr/local/lib/node_modules/ts-node/src/index.ts:350:18
    at Object.compile (/usr/local/lib/node_modules/ts-node/src/index.ts:509:17)
    at Module.m._compile (/usr/local/lib/node_modules/ts-node/src/index.ts:413:44)
    at Module._extensions..js (module.js:580:10)
    at Object.require.extensions.(anonymous function) [as .ts] (/usr/local/lib/node_modules/ts-node/src/index.ts:416:12)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Function.Module.runMain (module.js:605:10)

which is since Message.say signature is:

  public say(content: string, replyTo?: Contact|Contact[]): Promise<any> {

what is the interface to send a MediaMessage?

since export class MediaMessage extends Message {
I also tried sending a MediaMessage with an empty string.

    if (/^(img)$/i.test(m.content()) ) {
      let img = new MediaMessage('/image/wechaty-icon.png')
      img.say("")
      log.info('Bot', 'REPLY: dong')
    }

but gives error:

07:14:39 ERR Bot on(message) exception: SyntaxError: Unexpected token / in JSON at position 0

@dcsan

This comment has been minimized.

Show comment
Hide comment
@dcsan

dcsan Mar 28, 2017

UPDATE

I tried just doing a clone of @mukaiu 's fork, and that seems to work very well in sending an image message. So at this point I'm not sure about this PR but the forked repo is functional.

https://github.com/mukaiu/wechaty/blob/master/example/ding-dong-bot.ts#L75

So, I guess my method for reviewing this PR isn't working.
Just FYI since I am not a maintainer of this repo, it's a bit more complicated to check out a fork.

this message signature extensions did not come down for me

  public async say(textOrMedia: string | MediaMessage): Promise<void> {

even though it shows up in the PR
https://github.com/Chatie/wechaty/pull/337/files#diff-91d335b29ba8ed8ebb1d29260610d794R584

  git checkout master
  git pull
  git fetch origin pull/337/head

update

git pull origin pull/337/head

now seems to get the latest from this PR
but that gives a different error on running code

07:48:12 ERR PuppetWeb send() exception: cannot say nothing
(node:74270) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: cannot say nothing
(node:74270) DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

So anyway, mukais fork works fine. These problems are more about all the tooling/PRs/workflow on this project. I'll approve the PR as it doesn't introduce any new problems, and then do a clean checkout of the project where I can actually test this feature.

The other suggestion could be to make an unstable / edge branch and merge changes to that branch when these new features are being introduced.
Then you can get more people to easily test work in progress features.

dcsan commented Mar 28, 2017

UPDATE

I tried just doing a clone of @mukaiu 's fork, and that seems to work very well in sending an image message. So at this point I'm not sure about this PR but the forked repo is functional.

https://github.com/mukaiu/wechaty/blob/master/example/ding-dong-bot.ts#L75

So, I guess my method for reviewing this PR isn't working.
Just FYI since I am not a maintainer of this repo, it's a bit more complicated to check out a fork.

this message signature extensions did not come down for me

  public async say(textOrMedia: string | MediaMessage): Promise<void> {

even though it shows up in the PR
https://github.com/Chatie/wechaty/pull/337/files#diff-91d335b29ba8ed8ebb1d29260610d794R584

  git checkout master
  git pull
  git fetch origin pull/337/head

update

git pull origin pull/337/head

now seems to get the latest from this PR
but that gives a different error on running code

07:48:12 ERR PuppetWeb send() exception: cannot say nothing
(node:74270) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: cannot say nothing
(node:74270) DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

So anyway, mukais fork works fine. These problems are more about all the tooling/PRs/workflow on this project. I'll approve the PR as it doesn't introduce any new problems, and then do a clean checkout of the project where I can actually test this feature.

The other suggestion could be to make an unstable / edge branch and merge changes to that branch when these new features are being introduced.
Then you can get more people to easily test work in progress features.

@dcsan

dcsan approved these changes Mar 28, 2017

I can't get the checkout of the PR to work but the original fork is working very well. I'd like to get this into master (or an edge branch) asap so we can actually use it.

@dcsan

This comment has been minimized.

Show comment
Hide comment
@dcsan

dcsan Mar 28, 2017

if you did make an "edge" branch it would make it a lot easier for me to make a fork based off of that and start modifying / sending PRs. Making a PR based on someone else's PR on a repo that you don't own is just too much complication.

dcsan commented Mar 28, 2017

if you did make an "edge" branch it would make it a lot easier for me to make a fork based off of that and start modifying / sending PRs. Making a PR based on someone else's PR on a repo that you don't own is just too much complication.

@zixia zixia merged commit 11760a5 into Chatie:master Mar 30, 2017

4 of 6 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
coverage/coveralls Coverage decreased (-1.9%) to 54.561%
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 Mar 30, 2017

Member

Thanks @dcsan @lijiarui for approving this PR, it really helps.

@dcsan I like your idea of creating a edge branch to automaticaly merge the Pull Requests(which passed CI) for testers.

Do you know any web service or tools to do this kind of job(merge pull requests to a branch) automaticaly? That will be very efficiency.

Member

zixia commented Mar 30, 2017

Thanks @dcsan @lijiarui for approving this PR, it really helps.

@dcsan I like your idea of creating a edge branch to automaticaly merge the Pull Requests(which passed CI) for testers.

Do you know any web service or tools to do this kind of job(merge pull requests to a branch) automaticaly? That will be very efficiency.

@dcsan

This comment has been minimized.

Show comment
Hide comment
@dcsan

dcsan Mar 30, 2017

I'm not sure, but I dont think we have that many PRs now?
i have seen some other github projects with very active bots, so I will try to take a note next time I see this. Rails and many other projects keep edge branch.

dcsan commented Mar 30, 2017

I'm not sure, but I dont think we have that many PRs now?
i have seen some other github projects with very active bots, so I will try to take a note next time I see this. Rails and many other projects keep edge branch.

@dcsan

This comment has been minimized.

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