Making geom_jitter work

[13bzhang]

This is a pull request for Issue #221 "Conversion not implemented for geom_jitter".
Note: I am having trouble with getting the seed to set for the testthat.
https://github.com/ropensci/plotly/blob/baobao-geom_jitter/tests/testthat/test-ggplot-jitter.R

@mkcor@cpsievert@chriddyp

[cpsievert]

I carried over commits from #213 since that should be merged into master soon ( @chriddyp, not much has changed on #213 since you last 💃, but we're waiting for you to ;)

[13bzhang]

Ok, that was helpful! Thank you!

[13bzhang]

@cpsievert What do I need to do more on this issue?

[cpsievert]

@13bzhang the test that I added currently isn't passing, but that'd be great if you want to fix it!

[mkcor]

@cpsievert I'm trying to catch up on all the open PRs so at this stage I can't say (and I don't think) that something is wrong or bad code-wise, but I'm not so sure workflow-wise. I thought I had clarified our review pipeline before, but maybe not so clearly after all.

I carried over commits from #213 since that should be merged into master soon

A comment like this makes me nervous and we don't go by this practice. The only branch you want to merge into your WIP feature branch is master. Even then, I would typically prefer git rebase master over git merge master, because that leaves the commit history cleaner, but only do this if you're the only committer on your feature branch. So, use rebase with caution!

Also, open PRs don't form an ordered queue. Ideally, they would... But typically person1 may submit PR1 and that's pretty complex (requires substantial review work or whatever), meanwhile person2 submits PR2, and PR2 gets reviewed and +1'd before, so only then do you update the DESCRIPTION etc. info, then it gets merged into master... So then person1 may want to merge master into their branch that has the changes submitted via PR1... I mean, PR1 doesn't necessarily get merged before PR2 and there is no reason whatsoever to 'carry over' changes from PR1 into PR2...

[cpsievert]

@mkcor I agree, this is generally a good rule-of-thumb, but I disagree that there is no reason whatsoever for a pull request to branch off from another pull request (especially when the guideline is one feature/topic per pull request).

Say a feature in PR2 depends on PR1, it's much easier to branch off of PR1 rather than resolving conflicts after PR1 is merged/rebased with master.

@13bzhang for future reference, I don't really care if you merge or rebase, just do whatever works for you.

[13bzhang]

@cpsievert@mkcor I'm gonna get into this debate, hehe.
I'm looking into why the tests are failing.

[13bzhang]

@cpsievert@mkcor It turns out the bug was a silly mistake in the naming of the ggplot object. 😆 Travis CI passed!

[cpsievert]

The message below was automatically generated after build https://travis-ci.org/ropensci/plotly/builds/65027287

On TravisCI, commit 037a027 was successfully merged with fb40491 (master) to create ddf8d41. A visual testing table comparing fb40491 with ddf8d41 can be found here:
http://ropensci.github.io/plotly-test-table/tables/ddf8d411cbf137ff3c1d248255f1f5720d494c2d/index.html

[cpsievert]

Awesome, thanks for the fix @13bzhang! We should wait for @chriddyp's approval here. In the meantime, if you want, you could bump the version and update NEWS over on #214 (and merge it with master) since it has been ready for a while now...

[13bzhang]

Regarding #214, I have not done a merge on my own yet. Is there some particular procedure I should follow? Can you walk me through it?

[cpsievert]

Here is a good example of bumping the version and updating NEWS -> edf055b

After that, to merge, just click that green "merge pull request" button :)

[13bzhang]

It's too weird to talk about #214 on this PR. My brain hurts. Ok I will move the conversation over there.

[mkcor]

@cpsievert +1 on branching off from a branch that may not be master -- this wouldn't result in a merge in the history.

[cpsievert]

The message below was automatically generated after build https://travis-ci.org/ropensci/plotly/builds/96397958

Detected 3 differences ->

http://cpsievert.github.io/plotly-test-table/7217419/jitter-basic
http://cpsievert.github.io/plotly-test-table/7217419/scatterplots-geom_jitter
http://cpsievert.github.io/plotly-test-table/7217419/scatterplots-jitter

[13bzhang]

This is a pull request for Issue #221 "Conversion not implemented for geom_jitter".
Note: I am having trouble with getting the seed to set for the testthat.
https://github.com/ropensci/plotly/blob/baobao-geom_jitter/tests/testthat/test-ggplot-jitter.R

@mkcor@cpsievert@chriddyp

[cpsievert]

I carried over commits from #213 since that should be merged into master soon ( @chriddyp, not much has changed on #213 since you last 💃, but we're waiting for you to ;)

[13bzhang]

Ok, that was helpful! Thank you!

[13bzhang]

@cpsievert What do I need to do more on this issue?

[cpsievert]

@13bzhang the test that I added currently isn't passing, but that'd be great if you want to fix it!

[mkcor]

@cpsievert I'm trying to catch up on all the open PRs so at this stage I can't say (and I don't think) that something is wrong or bad code-wise, but I'm not so sure workflow-wise. I thought I had clarified our review pipeline before, but maybe not so clearly after all.

I carried over commits from #213 since that should be merged into master soon

A comment like this makes me nervous and we don't go by this practice. The only branch you want to merge into your WIP feature branch is master. Even then, I would typically prefer git rebase master over git merge master, because that leaves the commit history cleaner, but only do this if you're the only committer on your feature branch. So, use rebase with caution!

Also, open PRs don't form an ordered queue. Ideally, they would... But typically person1 may submit PR1 and that's pretty complex (requires substantial review work or whatever), meanwhile person2 submits PR2, and PR2 gets reviewed and +1'd before, so only then do you update the DESCRIPTION etc. info, then it gets merged into master... So then person1 may want to merge master into their branch that has the changes submitted via PR1... I mean, PR1 doesn't necessarily get merged before PR2 and there is no reason whatsoever to 'carry over' changes from PR1 into PR2...

[cpsievert]

@mkcor I agree, this is generally a good rule-of-thumb, but I disagree that there is no reason whatsoever for a pull request to branch off from another pull request (especially when the guideline is one feature/topic per pull request).

Say a feature in PR2 depends on PR1, it's much easier to branch off of PR1 rather than resolving conflicts after PR1 is merged/rebased with master.

@13bzhang for future reference, I don't really care if you merge or rebase, just do whatever works for you.

[13bzhang]

@cpsievert@mkcor I'm gonna get into this debate, hehe.
I'm looking into why the tests are failing.

[13bzhang]

@cpsievert@mkcor It turns out the bug was a silly mistake in the naming of the ggplot object. 😆 Travis CI passed!

[cpsievert]

The message below was automatically generated after build https://travis-ci.org/ropensci/plotly/builds/65027287

On TravisCI, commit 037a027 was successfully merged with fb40491 (master) to create ddf8d41. A visual testing table comparing fb40491 with ddf8d41 can be found here:
http://ropensci.github.io/plotly-test-table/tables/ddf8d411cbf137ff3c1d248255f1f5720d494c2d/index.html

[cpsievert]

Awesome, thanks for the fix @13bzhang! We should wait for @chriddyp's approval here. In the meantime, if you want, you could bump the version and update NEWS over on #214 (and merge it with master) since it has been ready for a while now...

[13bzhang]

Regarding #214, I have not done a merge on my own yet. Is there some particular procedure I should follow? Can you walk me through it?

[cpsievert]

Here is a good example of bumping the version and updating NEWS -> edf055b

After that, to merge, just click that green "merge pull request" button :)

[13bzhang]

It's too weird to talk about #214 on this PR. My brain hurts. Ok I will move the conversation over there.

[mkcor]

@cpsievert +1 on branching off from a branch that may not be master -- this wouldn't result in a merge in the history.

[cpsievert]

The message below was automatically generated after build https://travis-ci.org/ropensci/plotly/builds/96397958

Detected 3 differences ->

http://cpsievert.github.io/plotly-test-table/7217419/jitter-basic
http://cpsievert.github.io/plotly-test-table/7217419/scatterplots-geom_jitter
http://cpsievert.github.io/plotly-test-table/7217419/scatterplots-jitter

[13bzhang]

This is a pull request for Issue #221 "Conversion not implemented for geom_jitter".
Note: I am having trouble with getting the seed to set for the testthat.
https://github.com/ropensci/plotly/blob/baobao-geom_jitter/tests/testthat/test-ggplot-jitter.R

@mkcor@cpsievert@chriddyp

[cpsievert]

I carried over commits from #213 since that should be merged into master soon ( @chriddyp, not much has changed on #213 since you last 💃, but we're waiting for you to ;)

[13bzhang]

Ok, that was helpful! Thank you!

[13bzhang]

@cpsievert What do I need to do more on this issue?

[cpsievert]

@13bzhang the test that I added currently isn't passing, but that'd be great if you want to fix it!

[mkcor]

@cpsievert I'm trying to catch up on all the open PRs so at this stage I can't say (and I don't think) that something is wrong or bad code-wise, but I'm not so sure workflow-wise. I thought I had clarified our review pipeline before, but maybe not so clearly after all.

I carried over commits from #213 since that should be merged into master soon

A comment like this makes me nervous and we don't go by this practice. The only branch you want to merge into your WIP feature branch is master. Even then, I would typically prefer git rebase master over git merge master, because that leaves the commit history cleaner, but only do this if you're the only committer on your feature branch. So, use rebase with caution!

Also, open PRs don't form an ordered queue. Ideally, they would... But typically person1 may submit PR1 and that's pretty complex (requires substantial review work or whatever), meanwhile person2 submits PR2, and PR2 gets reviewed and +1'd before, so only then do you update the DESCRIPTION etc. info, then it gets merged into master... So then person1 may want to merge master into their branch that has the changes submitted via PR1... I mean, PR1 doesn't necessarily get merged before PR2 and there is no reason whatsoever to 'carry over' changes from PR1 into PR2...

[cpsievert]

@mkcor I agree, this is generally a good rule-of-thumb, but I disagree that there is no reason whatsoever for a pull request to branch off from another pull request (especially when the guideline is one feature/topic per pull request).

Say a feature in PR2 depends on PR1, it's much easier to branch off of PR1 rather than resolving conflicts after PR1 is merged/rebased with master.

@13bzhang for future reference, I don't really care if you merge or rebase, just do whatever works for you.

[13bzhang]

@cpsievert@mkcor I'm gonna get into this debate, hehe.
I'm looking into why the tests are failing.

[13bzhang]

@cpsievert@mkcor It turns out the bug was a silly mistake in the naming of the ggplot object. 😆 Travis CI passed!

[cpsievert]

The message below was automatically generated after build https://travis-ci.org/ropensci/plotly/builds/65027287

On TravisCI, commit 037a027 was successfully merged with fb40491 (master) to create ddf8d41. A visual testing table comparing fb40491 with ddf8d41 can be found here:
http://ropensci.github.io/plotly-test-table/tables/ddf8d411cbf137ff3c1d248255f1f5720d494c2d/index.html

[cpsievert]

Awesome, thanks for the fix @13bzhang! We should wait for @chriddyp's approval here. In the meantime, if you want, you could bump the version and update NEWS over on #214 (and merge it with master) since it has been ready for a while now...

[13bzhang]

Regarding #214, I have not done a merge on my own yet. Is there some particular procedure I should follow? Can you walk me through it?

[cpsievert]

Here is a good example of bumping the version and updating NEWS -> edf055b

After that, to merge, just click that green "merge pull request" button :)

[13bzhang]

It's too weird to talk about #214 on this PR. My brain hurts. Ok I will move the conversation over there.

[mkcor]

@cpsievert +1 on branching off from a branch that may not be master -- this wouldn't result in a merge in the history.

[cpsievert]

The message below was automatically generated after build https://travis-ci.org/ropensci/plotly/builds/96397958

Detected 3 differences ->

http://cpsievert.github.io/plotly-test-table/7217419/jitter-basic
http://cpsievert.github.io/plotly-test-table/7217419/scatterplots-geom_jitter
http://cpsievert.github.io/plotly-test-table/7217419/scatterplots-jitter

[13bzhang]

This is a pull request for Issue #221 "Conversion not implemented for geom_jitter".
Note: I am having trouble with getting the seed to set for the testthat.
https://github.com/ropensci/plotly/blob/baobao-geom_jitter/tests/testthat/test-ggplot-jitter.R

@mkcor@cpsievert@chriddyp

[cpsievert]

I carried over commits from #213 since that should be merged into master soon ( @chriddyp, not much has changed on #213 since you last 💃, but we're waiting for you to ;)

[13bzhang]

Ok, that was helpful! Thank you!

[13bzhang]

@cpsievert What do I need to do more on this issue?

[cpsievert]

@13bzhang the test that I added currently isn't passing, but that'd be great if you want to fix it!

[mkcor]

@cpsievert I'm trying to catch up on all the open PRs so at this stage I can't say (and I don't think) that something is wrong or bad code-wise, but I'm not so sure workflow-wise. I thought I had clarified our review pipeline before, but maybe not so clearly after all.

I carried over commits from #213 since that should be merged into master soon

A comment like this makes me nervous and we don't go by this practice. The only branch you want to merge into your WIP feature branch is master. Even then, I would typically prefer git rebase master over git merge master, because that leaves the commit history cleaner, but only do this if you're the only committer on your feature branch. So, use rebase with caution!

Also, open PRs don't form an ordered queue. Ideally, they would... But typically person1 may submit PR1 and that's pretty complex (requires substantial review work or whatever), meanwhile person2 submits PR2, and PR2 gets reviewed and +1'd before, so only then do you update the DESCRIPTION etc. info, then it gets merged into master... So then person1 may want to merge master into their branch that has the changes submitted via PR1... I mean, PR1 doesn't necessarily get merged before PR2 and there is no reason whatsoever to 'carry over' changes from PR1 into PR2...

[cpsievert]

@mkcor I agree, this is generally a good rule-of-thumb, but I disagree that there is no reason whatsoever for a pull request to branch off from another pull request (especially when the guideline is one feature/topic per pull request).

Say a feature in PR2 depends on PR1, it's much easier to branch off of PR1 rather than resolving conflicts after PR1 is merged/rebased with master.

@13bzhang for future reference, I don't really care if you merge or rebase, just do whatever works for you.

[13bzhang]

@cpsievert@mkcor I'm gonna get into this debate, hehe.
I'm looking into why the tests are failing.

[13bzhang]

@cpsievert@mkcor It turns out the bug was a silly mistake in the naming of the ggplot object. 😆 Travis CI passed!

[cpsievert]

The message below was automatically generated after build https://travis-ci.org/ropensci/plotly/builds/65027287

On TravisCI, commit 037a027 was successfully merged with fb40491 (master) to create ddf8d41. A visual testing table comparing fb40491 with ddf8d41 can be found here:
http://ropensci.github.io/plotly-test-table/tables/ddf8d411cbf137ff3c1d248255f1f5720d494c2d/index.html

[cpsievert]

Awesome, thanks for the fix @13bzhang! We should wait for @chriddyp's approval here. In the meantime, if you want, you could bump the version and update NEWS over on #214 (and merge it with master) since it has been ready for a while now...

[13bzhang]

Regarding #214, I have not done a merge on my own yet. Is there some particular procedure I should follow? Can you walk me through it?

[cpsievert]

Here is a good example of bumping the version and updating NEWS -> edf055b

After that, to merge, just click that green "merge pull request" button :)

[13bzhang]

It's too weird to talk about #214 on this PR. My brain hurts. Ok I will move the conversation over there.

[mkcor]

@cpsievert +1 on branching off from a branch that may not be master -- this wouldn't result in a merge in the history.

[cpsievert]

The message below was automatically generated after build https://travis-ci.org/ropensci/plotly/builds/96397958

Detected 3 differences ->

http://cpsievert.github.io/plotly-test-table/7217419/jitter-basic
http://cpsievert.github.io/plotly-test-table/7217419/scatterplots-geom_jitter
http://cpsievert.github.io/plotly-test-table/7217419/scatterplots-jitter

[13bzhang]

This is a pull request for Issue #221 "Conversion not implemented for geom_jitter".
Note: I am having trouble with getting the seed to set for the testthat.
https://github.com/ropensci/plotly/blob/baobao-geom_jitter/tests/testthat/test-ggplot-jitter.R

@mkcor@cpsievert@chriddyp

[cpsievert]

I carried over commits from #213 since that should be merged into master soon ( @chriddyp, not much has changed on #213 since you last 💃, but we're waiting for you to ;)

[13bzhang]

Ok, that was helpful! Thank you!

[13bzhang]

@cpsievert What do I need to do more on this issue?

[cpsievert]

@13bzhang the test that I added currently isn't passing, but that'd be great if you want to fix it!

[mkcor]

@cpsievert I'm trying to catch up on all the open PRs so at this stage I can't say (and I don't think) that something is wrong or bad code-wise, but I'm not so sure workflow-wise. I thought I had clarified our review pipeline before, but maybe not so clearly after all.

I carried over commits from #213 since that should be merged into master soon

A comment like this makes me nervous and we don't go by this practice. The only branch you want to merge into your WIP feature branch is master. Even then, I would typically prefer git rebase master over git merge master, because that leaves the commit history cleaner, but only do this if you're the only committer on your feature branch. So, use rebase with caution!

Also, open PRs don't form an ordered queue. Ideally, they would... But typically person1 may submit PR1 and that's pretty complex (requires substantial review work or whatever), meanwhile person2 submits PR2, and PR2 gets reviewed and +1'd before, so only then do you update the DESCRIPTION etc. info, then it gets merged into master... So then person1 may want to merge master into their branch that has the changes submitted via PR1... I mean, PR1 doesn't necessarily get merged before PR2 and there is no reason whatsoever to 'carry over' changes from PR1 into PR2...

[cpsievert]

@mkcor I agree, this is generally a good rule-of-thumb, but I disagree that there is no reason whatsoever for a pull request to branch off from another pull request (especially when the guideline is one feature/topic per pull request).

Say a feature in PR2 depends on PR1, it's much easier to branch off of PR1 rather than resolving conflicts after PR1 is merged/rebased with master.

@13bzhang for future reference, I don't really care if you merge or rebase, just do whatever works for you.

[13bzhang]

@cpsievert@mkcor I'm gonna get into this debate, hehe.
I'm looking into why the tests are failing.

[13bzhang]

@cpsievert@mkcor It turns out the bug was a silly mistake in the naming of the ggplot object. 😆 Travis CI passed!

[cpsievert]

The message below was automatically generated after build https://travis-ci.org/ropensci/plotly/builds/65027287

On TravisCI, commit 037a027 was successfully merged with fb40491 (master) to create ddf8d41. A visual testing table comparing fb40491 with ddf8d41 can be found here:
http://ropensci.github.io/plotly-test-table/tables/ddf8d411cbf137ff3c1d248255f1f5720d494c2d/index.html

[cpsievert]

Awesome, thanks for the fix @13bzhang! We should wait for @chriddyp's approval here. In the meantime, if you want, you could bump the version and update NEWS over on #214 (and merge it with master) since it has been ready for a while now...

[13bzhang]

Regarding #214, I have not done a merge on my own yet. Is there some particular procedure I should follow? Can you walk me through it?

[cpsievert]

Here is a good example of bumping the version and updating NEWS -> edf055b

After that, to merge, just click that green "merge pull request" button :)

[13bzhang]

It's too weird to talk about #214 on this PR. My brain hurts. Ok I will move the conversation over there.

[mkcor]

@cpsievert +1 on branching off from a branch that may not be master -- this wouldn't result in a merge in the history.

[cpsievert]

The message below was automatically generated after build https://travis-ci.org/ropensci/plotly/builds/96397958

Detected 3 differences ->

http://cpsievert.github.io/plotly-test-table/7217419/jitter-basic
http://cpsievert.github.io/plotly-test-table/7217419/scatterplots-geom_jitter
http://cpsievert.github.io/plotly-test-table/7217419/scatterplots-jitter

[13bzhang]

This is a pull request for Issue #221 "Conversion not implemented for geom_jitter".
Note: I am having trouble with getting the seed to set for the testthat.
https://github.com/ropensci/plotly/blob/baobao-geom_jitter/tests/testthat/test-ggplot-jitter.R

@mkcor@cpsievert@chriddyp

[cpsievert]

I carried over commits from #213 since that should be merged into master soon ( @chriddyp, not much has changed on #213 since you last 💃, but we're waiting for you to ;)

[13bzhang]

Ok, that was helpful! Thank you!

[13bzhang]

@cpsievert What do I need to do more on this issue?

[cpsievert]

@13bzhang the test that I added currently isn't passing, but that'd be great if you want to fix it!

[mkcor]

@cpsievert I'm trying to catch up on all the open PRs so at this stage I can't say (and I don't think) that something is wrong or bad code-wise, but I'm not so sure workflow-wise. I thought I had clarified our review pipeline before, but maybe not so clearly after all.

I carried over commits from #213 since that should be merged into master soon

A comment like this makes me nervous and we don't go by this practice. The only branch you want to merge into your WIP feature branch is master. Even then, I would typically prefer git rebase master over git merge master, because that leaves the commit history cleaner, but only do this if you're the only committer on your feature branch. So, use rebase with caution!

Also, open PRs don't form an ordered queue. Ideally, they would... But typically person1 may submit PR1 and that's pretty complex (requires substantial review work or whatever), meanwhile person2 submits PR2, and PR2 gets reviewed and +1'd before, so only then do you update the DESCRIPTION etc. info, then it gets merged into master... So then person1 may want to merge master into their branch that has the changes submitted via PR1... I mean, PR1 doesn't necessarily get merged before PR2 and there is no reason whatsoever to 'carry over' changes from PR1 into PR2...

[cpsievert]

@mkcor I agree, this is generally a good rule-of-thumb, but I disagree that there is no reason whatsoever for a pull request to branch off from another pull request (especially when the guideline is one feature/topic per pull request).

Say a feature in PR2 depends on PR1, it's much easier to branch off of PR1 rather than resolving conflicts after PR1 is merged/rebased with master.

@13bzhang for future reference, I don't really care if you merge or rebase, just do whatever works for you.

[13bzhang]

@cpsievert@mkcor I'm gonna get into this debate, hehe.
I'm looking into why the tests are failing.

[13bzhang]

@cpsievert@mkcor It turns out the bug was a silly mistake in the naming of the ggplot object. 😆 Travis CI passed!

[cpsievert]

The message below was automatically generated after build https://travis-ci.org/ropensci/plotly/builds/65027287

On TravisCI, commit 037a027 was successfully merged with fb40491 (master) to create ddf8d41. A visual testing table comparing fb40491 with ddf8d41 can be found here:
http://ropensci.github.io/plotly-test-table/tables/ddf8d411cbf137ff3c1d248255f1f5720d494c2d/index.html

[cpsievert]

Awesome, thanks for the fix @13bzhang! We should wait for @chriddyp's approval here. In the meantime, if you want, you could bump the version and update NEWS over on #214 (and merge it with master) since it has been ready for a while now...

[13bzhang]

Regarding #214, I have not done a merge on my own yet. Is there some particular procedure I should follow? Can you walk me through it?

[cpsievert]

Here is a good example of bumping the version and updating NEWS -> edf055b

After that, to merge, just click that green "merge pull request" button :)

[13bzhang]

It's too weird to talk about #214 on this PR. My brain hurts. Ok I will move the conversation over there.

[mkcor]

@cpsievert +1 on branching off from a branch that may not be master -- this wouldn't result in a merge in the history.

[cpsievert]

The message below was automatically generated after build https://travis-ci.org/ropensci/plotly/builds/96397958

Detected 3 differences ->

http://cpsievert.github.io/plotly-test-table/7217419/jitter-basic
http://cpsievert.github.io/plotly-test-table/7217419/scatterplots-geom_jitter
http://cpsievert.github.io/plotly-test-table/7217419/scatterplots-jitter

[13bzhang]

This is a pull request for Issue #221 "Conversion not implemented for geom_jitter".
Note: I am having trouble with getting the seed to set for the testthat.
https://github.com/ropensci/plotly/blob/baobao-geom_jitter/tests/testthat/test-ggplot-jitter.R

@mkcor@cpsievert@chriddyp

[cpsievert]

I carried over commits from #213 since that should be merged into master soon ( @chriddyp, not much has changed on #213 since you last 💃, but we're waiting for you to ;)

[13bzhang]

Ok, that was helpful! Thank you!

[13bzhang]

@cpsievert What do I need to do more on this issue?

[cpsievert]

@13bzhang the test that I added currently isn't passing, but that'd be great if you want to fix it!

[mkcor]

@cpsievert I'm trying to catch up on all the open PRs so at this stage I can't say (and I don't think) that something is wrong or bad code-wise, but I'm not so sure workflow-wise. I thought I had clarified our review pipeline before, but maybe not so clearly after all.

I carried over commits from #213 since that should be merged into master soon

A comment like this makes me nervous and we don't go by this practice. The only branch you want to merge into your WIP feature branch is master. Even then, I would typically prefer git rebase master over git merge master, because that leaves the commit history cleaner, but only do this if you're the only committer on your feature branch. So, use rebase with caution!

Also, open PRs don't form an ordered queue. Ideally, they would... But typically person1 may submit PR1 and that's pretty complex (requires substantial review work or whatever), meanwhile person2 submits PR2, and PR2 gets reviewed and +1'd before, so only then do you update the DESCRIPTION etc. info, then it gets merged into master... So then person1 may want to merge master into their branch that has the changes submitted via PR1... I mean, PR1 doesn't necessarily get merged before PR2 and there is no reason whatsoever to 'carry over' changes from PR1 into PR2...

[cpsievert]

@mkcor I agree, this is generally a good rule-of-thumb, but I disagree that there is no reason whatsoever for a pull request to branch off from another pull request (especially when the guideline is one feature/topic per pull request).

Say a feature in PR2 depends on PR1, it's much easier to branch off of PR1 rather than resolving conflicts after PR1 is merged/rebased with master.

@13bzhang for future reference, I don't really care if you merge or rebase, just do whatever works for you.

[13bzhang]

@cpsievert@mkcor I'm gonna get into this debate, hehe.
I'm looking into why the tests are failing.

[13bzhang]

@cpsievert@mkcor It turns out the bug was a silly mistake in the naming of the ggplot object. 😆 Travis CI passed!

[cpsievert]

The message below was automatically generated after build https://travis-ci.org/ropensci/plotly/builds/65027287

On TravisCI, commit 037a027 was successfully merged with fb40491 (master) to create ddf8d41. A visual testing table comparing fb40491 with ddf8d41 can be found here:
http://ropensci.github.io/plotly-test-table/tables/ddf8d411cbf137ff3c1d248255f1f5720d494c2d/index.html

[cpsievert]

Awesome, thanks for the fix @13bzhang! We should wait for @chriddyp's approval here. In the meantime, if you want, you could bump the version and update NEWS over on #214 (and merge it with master) since it has been ready for a while now...

[13bzhang]

Regarding #214, I have not done a merge on my own yet. Is there some particular procedure I should follow? Can you walk me through it?

[cpsievert]

Here is a good example of bumping the version and updating NEWS -> edf055b

After that, to merge, just click that green "merge pull request" button :)

[13bzhang]

It's too weird to talk about #214 on this PR. My brain hurts. Ok I will move the conversation over there.

[mkcor]

@cpsievert +1 on branching off from a branch that may not be master -- this wouldn't result in a merge in the history.

[cpsievert]

The message below was automatically generated after build https://travis-ci.org/ropensci/plotly/builds/96397958

Detected 3 differences ->

http://cpsievert.github.io/plotly-test-table/7217419/jitter-basic
http://cpsievert.github.io/plotly-test-table/7217419/scatterplots-geom_jitter
http://cpsievert.github.io/plotly-test-table/7217419/scatterplots-jitter

[13bzhang]

This is a pull request for Issue #221 "Conversion not implemented for geom_jitter".
Note: I am having trouble with getting the seed to set for the testthat.
https://github.com/ropensci/plotly/blob/baobao-geom_jitter/tests/testthat/test-ggplot-jitter.R

@mkcor@cpsievert@chriddyp

[cpsievert]

I carried over commits from #213 since that should be merged into master soon ( @chriddyp, not much has changed on #213 since you last 💃, but we're waiting for you to ;)

[13bzhang]

Ok, that was helpful! Thank you!

[13bzhang]

@cpsievert What do I need to do more on this issue?

[cpsievert]

@13bzhang the test that I added currently isn't passing, but that'd be great if you want to fix it!

[mkcor]

@cpsievert I'm trying to catch up on all the open PRs so at this stage I can't say (and I don't think) that something is wrong or bad code-wise, but I'm not so sure workflow-wise. I thought I had clarified our review pipeline before, but maybe not so clearly after all.

I carried over commits from #213 since that should be merged into master soon

A comment like this makes me nervous and we don't go by this practice. The only branch you want to merge into your WIP feature branch is master. Even then, I would typically prefer git rebase master over git merge master, because that leaves the commit history cleaner, but only do this if you're the only committer on your feature branch. So, use rebase with caution!

Also, open PRs don't form an ordered queue. Ideally, they would... But typically person1 may submit PR1 and that's pretty complex (requires substantial review work or whatever), meanwhile person2 submits PR2, and PR2 gets reviewed and +1'd before, so only then do you update the DESCRIPTION etc. info, then it gets merged into master... So then person1 may want to merge master into their branch that has the changes submitted via PR1... I mean, PR1 doesn't necessarily get merged before PR2 and there is no reason whatsoever to 'carry over' changes from PR1 into PR2...

[cpsievert]

@mkcor I agree, this is generally a good rule-of-thumb, but I disagree that there is no reason whatsoever for a pull request to branch off from another pull request (especially when the guideline is one feature/topic per pull request).

Say a feature in PR2 depends on PR1, it's much easier to branch off of PR1 rather than resolving conflicts after PR1 is merged/rebased with master.

@13bzhang for future reference, I don't really care if you merge or rebase, just do whatever works for you.

[13bzhang]

@cpsievert@mkcor I'm gonna get into this debate, hehe.
I'm looking into why the tests are failing.

[13bzhang]

@cpsievert@mkcor It turns out the bug was a silly mistake in the naming of the ggplot object. 😆 Travis CI passed!

[cpsievert]

The message below was automatically generated after build https://travis-ci.org/ropensci/plotly/builds/65027287

On TravisCI, commit 037a027 was successfully merged with fb40491 (master) to create ddf8d41. A visual testing table comparing fb40491 with ddf8d41 can be found here:
http://ropensci.github.io/plotly-test-table/tables/ddf8d411cbf137ff3c1d248255f1f5720d494c2d/index.html

[cpsievert]

Awesome, thanks for the fix @13bzhang! We should wait for @chriddyp's approval here. In the meantime, if you want, you could bump the version and update NEWS over on #214 (and merge it with master) since it has been ready for a while now...

[13bzhang]

Regarding #214, I have not done a merge on my own yet. Is there some particular procedure I should follow? Can you walk me through it?

[cpsievert]

Here is a good example of bumping the version and updating NEWS -> edf055b

After that, to merge, just click that green "merge pull request" button :)

[13bzhang]

It's too weird to talk about #214 on this PR. My brain hurts. Ok I will move the conversation over there.

[mkcor]

@cpsievert +1 on branching off from a branch that may not be master -- this wouldn't result in a merge in the history.

[cpsievert]

The message below was automatically generated after build https://travis-ci.org/ropensci/plotly/builds/96397958

Detected 3 differences ->

http://cpsievert.github.io/plotly-test-table/7217419/jitter-basic
http://cpsievert.github.io/plotly-test-table/7217419/scatterplots-geom_jitter
http://cpsievert.github.io/plotly-test-table/7217419/scatterplots-jitter