Fixes for error bars and ticks

[tdhock]

Hey guys, I coded some fixes for geom_errorbar and geom_errorbarh, which is the first problem in #161

I changed the way traces are merged at the end of gg2list, so now it does not matter if the error bars come before or after the other geoms (points, lines).

I also moved the definition of a few constants from corresp_one_one.R to the top of ggplotly.R -- this saves me time during development and debugging since I only have to source("ggplotly.R") instead of both code files (in the correct order). I hope this is not a big deal for you guys.

Tests pass on my machine.

[mkcor]

Thanks, @tdhock -- Before I actually review...

moved the definition of a few constants from corresp_one_one.R to the top of ggplotly.R

Nooooooooooooooooo!!! We are trying to modularize this code base.

You shouldn't only source("ggplotly.R") as you develop, you need the entire package to test properly that your changes are working. Typically, you should use a Makefile like:

all: build
R --interactive < test_script.R
build:
R CMD build /dir/to/your/repo/plotly
R CMD INSTALL --library=/dir/to/your/wip/lib plotly_version#.tar.gz

where test_script.R contains, say, a new conversion you're working on, so at the end you can make sure that the whole

$ make

runs successfully. As you develop, you probably insert some browser() calls here and there. So then you should

$ make build

Make sure your .Rhistory is never saved. Make sure your WIP version of "plotly" gets installed at the right place (not the default library for R packages). Your test_script.R should typically start with

require(plotly, lib.loc="/dir/to/your/wip/lib")

Then launch R and source("test_script.R"). Finally, to check that the test suite runs well,

$ cd tests
$ R

and, in the R console,

require(plotly, lib.loc="/dir/to/your/wip/lib")
source("testthat.R")

Cool, now this is documented somewhere.

[tdhock]

ok sorry about that, I moved the constants back to the other file.

[tdhock]

I have also included a bunch of tests from https://github.com/chriddyp/ggplot2-plotly-cookbook/blob/master/axes.R#L70-L71 which I will eventually make pass.

[mkcor]

Test-driven development, yes!

[mkcor]

Remove trailing white line.

[mkcor]

I wish this PR was split into 10 PRs... Would make the review process faster and safer.

[tdhock]

The updated test table shows the most recent version of this branch as toby.fixes.reviewed

http://ropensci.github.io/plotly-test-table/tables/c47b8026a456a0ae382011db8ac4cce074fe5b1d/index.html

everything looks fine to me, ready to merge if you guys are.

[mkcor]

I'm happy with the changes so far! If I find something 'missing', I'll submit a PR for you to review @tdhock ;)
👍
/cc @chriddyp

[chriddyp]

@tdhock I noticed this ticks-flip was fixed but then not here:

[mkcor]

@chriddyp Good catch! But I think we can leave this fix for another/different/upcoming PR, since this one is huge already. (@tdhock Do you append '.bug' when running the image test suite on a feature branch, and then '.reviewed' when you re-run it on that same branch once the code has been reviewed?)

[mkcor]

I think I'll increment the middle digit in version number, after this PR is merged!
On the contrary, fixing ticks-flip will only increment the last digit.

[tdhock]

@chriddyp turns out my plotly code was correct for that flipped example,

http://ropensci.github.io/plotly-test-table/tables/c47b8026a456a0ae382011db8ac4cce074fe5b1d/ticks-flip.html

but the plotly-test-table code was not. In particular the plotly filename in the JSON kwargs was the same for different versions of the ggplotly code, so actually the non-flipped image that showed up was made using the code from Carson's branch, and the plotly server did not update that image file right away.

But now for the test table I give the the SHA1 in the plotly filename so I hope the plotly server will respond back with a unique plotly every time. Is that a correct interpretation of the filename argument?

https://github.com/ropensci/plotly-test-table/blob/6a034a3a65e43c03e76072a75118390f998933ed/index.R#L179

@mkcor for the test table I assign arbitrary column labels by typing them into

https://github.com/ropensci/plotly-test-table/blob/gh-pages/code_commits.csv

so yes I was using "bug" to indicate a previous version and the "reviewed" to indicate the newer version. Is that OK or would you prefer that I use labels such as "new" and "old" ?

So anyways I corrected the test table and so I guess this branch is ready to merge. Can I please get a +1?

[mkcor]

I gave you a 👍 a long time ago because you wouldn't do the changes I would ask for so I thought I'll just do them myself...
Can you please remove files R/print.R, tests/testthat.R, and tests/testthat/test-ggplot-area.R from the changeset? They have nothing to do in this PR!

[tdhock]

Sure I can remove those files, if you tell me how?

[tdhock]

sorry I am a bit confused. should I just make the reverse changes and then commit them? or is there some other git command?

[tdhock]

And I'm sorry if I did not do all the changes you asked for --- I tried to address all of your comments.

[mkcor]

Well, I could have been more proactive myself and pulled the branch a while ago. Let me do it now and I'll share my recommendations.

[mkcor]

So, I couldn't revert commits, because these dummy changes occurred in the midst of other stuff, i.e., adcc2ba or 4766545 (hence my suspicion against git commit -a).

Actually I had pulled your branch before, but never pushed. So I did:

git checkout toby-fixes
git pull origin toby-fixes
git rm R/print.R git commit -m "Remove funny file"# Looked up id/hash for latest commit on master
git checkout 1d75115f719082 tests/testthat.R
git add tests/testthat.R git commit -m "Revert testthat.R file to latest commit on master"
git checkout 1d75115f719082 tests/testthat/test-ggplot-area.R
git add tests/testthat/test-ggplot-area.R git commit -m "Revert unrelated test file to latest commit on master"
git push origin toby-fixes

Hope you find this inspiring!

[mkcor]

+1

[tdhock]

Hey guys, I coded some fixes for geom_errorbar and geom_errorbarh, which is the first problem in #161

I changed the way traces are merged at the end of gg2list, so now it does not matter if the error bars come before or after the other geoms (points, lines).

I also moved the definition of a few constants from corresp_one_one.R to the top of ggplotly.R -- this saves me time during development and debugging since I only have to source("ggplotly.R") instead of both code files (in the correct order). I hope this is not a big deal for you guys.

Tests pass on my machine.

[mkcor]

Thanks, @tdhock -- Before I actually review...

moved the definition of a few constants from corresp_one_one.R to the top of ggplotly.R

Nooooooooooooooooo!!! We are trying to modularize this code base.

You shouldn't only source("ggplotly.R") as you develop, you need the entire package to test properly that your changes are working. Typically, you should use a Makefile like:

all: build
R --interactive < test_script.R
build:
R CMD build /dir/to/your/repo/plotly
R CMD INSTALL --library=/dir/to/your/wip/lib plotly_version#.tar.gz

where test_script.R contains, say, a new conversion you're working on, so at the end you can make sure that the whole

$ make

runs successfully. As you develop, you probably insert some browser() calls here and there. So then you should

$ make build

Make sure your .Rhistory is never saved. Make sure your WIP version of "plotly" gets installed at the right place (not the default library for R packages). Your test_script.R should typically start with

require(plotly, lib.loc="/dir/to/your/wip/lib")

Then launch R and source("test_script.R"). Finally, to check that the test suite runs well,

$ cd tests
$ R

and, in the R console,

require(plotly, lib.loc="/dir/to/your/wip/lib")
source("testthat.R")

Cool, now this is documented somewhere.

[tdhock]

ok sorry about that, I moved the constants back to the other file.

[tdhock]

I have also included a bunch of tests from https://github.com/chriddyp/ggplot2-plotly-cookbook/blob/master/axes.R#L70-L71 which I will eventually make pass.

[mkcor]

Test-driven development, yes!

[mkcor]

Remove trailing white line.

[mkcor]

I wish this PR was split into 10 PRs... Would make the review process faster and safer.

[tdhock]

The updated test table shows the most recent version of this branch as toby.fixes.reviewed

http://ropensci.github.io/plotly-test-table/tables/c47b8026a456a0ae382011db8ac4cce074fe5b1d/index.html

everything looks fine to me, ready to merge if you guys are.

[mkcor]

I'm happy with the changes so far! If I find something 'missing', I'll submit a PR for you to review @tdhock ;)
👍
/cc @chriddyp

[chriddyp]

@tdhock I noticed this ticks-flip was fixed but then not here:

[mkcor]

@chriddyp Good catch! But I think we can leave this fix for another/different/upcoming PR, since this one is huge already. (@tdhock Do you append '.bug' when running the image test suite on a feature branch, and then '.reviewed' when you re-run it on that same branch once the code has been reviewed?)

[mkcor]

I think I'll increment the middle digit in version number, after this PR is merged!
On the contrary, fixing ticks-flip will only increment the last digit.

[tdhock]

@chriddyp turns out my plotly code was correct for that flipped example,

http://ropensci.github.io/plotly-test-table/tables/c47b8026a456a0ae382011db8ac4cce074fe5b1d/ticks-flip.html

but the plotly-test-table code was not. In particular the plotly filename in the JSON kwargs was the same for different versions of the ggplotly code, so actually the non-flipped image that showed up was made using the code from Carson's branch, and the plotly server did not update that image file right away.

But now for the test table I give the the SHA1 in the plotly filename so I hope the plotly server will respond back with a unique plotly every time. Is that a correct interpretation of the filename argument?

https://github.com/ropensci/plotly-test-table/blob/6a034a3a65e43c03e76072a75118390f998933ed/index.R#L179

@mkcor for the test table I assign arbitrary column labels by typing them into

https://github.com/ropensci/plotly-test-table/blob/gh-pages/code_commits.csv

so yes I was using "bug" to indicate a previous version and the "reviewed" to indicate the newer version. Is that OK or would you prefer that I use labels such as "new" and "old" ?

So anyways I corrected the test table and so I guess this branch is ready to merge. Can I please get a +1?

[mkcor]

I gave you a 👍 a long time ago because you wouldn't do the changes I would ask for so I thought I'll just do them myself...
Can you please remove files R/print.R, tests/testthat.R, and tests/testthat/test-ggplot-area.R from the changeset? They have nothing to do in this PR!

[tdhock]

Sure I can remove those files, if you tell me how?

[tdhock]

sorry I am a bit confused. should I just make the reverse changes and then commit them? or is there some other git command?

[tdhock]

And I'm sorry if I did not do all the changes you asked for --- I tried to address all of your comments.

[mkcor]

Well, I could have been more proactive myself and pulled the branch a while ago. Let me do it now and I'll share my recommendations.

[mkcor]

So, I couldn't revert commits, because these dummy changes occurred in the midst of other stuff, i.e., adcc2ba or 4766545 (hence my suspicion against git commit -a).

Actually I had pulled your branch before, but never pushed. So I did:

git checkout toby-fixes
git pull origin toby-fixes
git rm R/print.R git commit -m "Remove funny file"# Looked up id/hash for latest commit on master
git checkout 1d75115f719082 tests/testthat.R
git add tests/testthat.R git commit -m "Revert testthat.R file to latest commit on master"
git checkout 1d75115f719082 tests/testthat/test-ggplot-area.R
git add tests/testthat/test-ggplot-area.R git commit -m "Revert unrelated test file to latest commit on master"
git push origin toby-fixes

Hope you find this inspiring!

[mkcor]

+1

[tdhock]

Hey guys, I coded some fixes for geom_errorbar and geom_errorbarh, which is the first problem in #161

I changed the way traces are merged at the end of gg2list, so now it does not matter if the error bars come before or after the other geoms (points, lines).

I also moved the definition of a few constants from corresp_one_one.R to the top of ggplotly.R -- this saves me time during development and debugging since I only have to source("ggplotly.R") instead of both code files (in the correct order). I hope this is not a big deal for you guys.

Tests pass on my machine.

[mkcor]

Thanks, @tdhock -- Before I actually review...

moved the definition of a few constants from corresp_one_one.R to the top of ggplotly.R

Nooooooooooooooooo!!! We are trying to modularize this code base.

You shouldn't only source("ggplotly.R") as you develop, you need the entire package to test properly that your changes are working. Typically, you should use a Makefile like:

all: build
R --interactive < test_script.R
build:
R CMD build /dir/to/your/repo/plotly
R CMD INSTALL --library=/dir/to/your/wip/lib plotly_version#.tar.gz

where test_script.R contains, say, a new conversion you're working on, so at the end you can make sure that the whole

$ make

runs successfully. As you develop, you probably insert some browser() calls here and there. So then you should

$ make build

Make sure your .Rhistory is never saved. Make sure your WIP version of "plotly" gets installed at the right place (not the default library for R packages). Your test_script.R should typically start with

require(plotly, lib.loc="/dir/to/your/wip/lib")

Then launch R and source("test_script.R"). Finally, to check that the test suite runs well,

$ cd tests
$ R

and, in the R console,

require(plotly, lib.loc="/dir/to/your/wip/lib")
source("testthat.R")

Cool, now this is documented somewhere.

[tdhock]

ok sorry about that, I moved the constants back to the other file.

[tdhock]

I have also included a bunch of tests from https://github.com/chriddyp/ggplot2-plotly-cookbook/blob/master/axes.R#L70-L71 which I will eventually make pass.

[mkcor]

Test-driven development, yes!

[mkcor]

Remove trailing white line.

[mkcor]

I wish this PR was split into 10 PRs... Would make the review process faster and safer.

[tdhock]

The updated test table shows the most recent version of this branch as toby.fixes.reviewed

http://ropensci.github.io/plotly-test-table/tables/c47b8026a456a0ae382011db8ac4cce074fe5b1d/index.html

everything looks fine to me, ready to merge if you guys are.

[mkcor]

I'm happy with the changes so far! If I find something 'missing', I'll submit a PR for you to review @tdhock ;)
👍
/cc @chriddyp

[chriddyp]

@tdhock I noticed this ticks-flip was fixed but then not here:

[mkcor]

@chriddyp Good catch! But I think we can leave this fix for another/different/upcoming PR, since this one is huge already. (@tdhock Do you append '.bug' when running the image test suite on a feature branch, and then '.reviewed' when you re-run it on that same branch once the code has been reviewed?)

[mkcor]

I think I'll increment the middle digit in version number, after this PR is merged!
On the contrary, fixing ticks-flip will only increment the last digit.

[tdhock]

@chriddyp turns out my plotly code was correct for that flipped example,

http://ropensci.github.io/plotly-test-table/tables/c47b8026a456a0ae382011db8ac4cce074fe5b1d/ticks-flip.html

but the plotly-test-table code was not. In particular the plotly filename in the JSON kwargs was the same for different versions of the ggplotly code, so actually the non-flipped image that showed up was made using the code from Carson's branch, and the plotly server did not update that image file right away.

But now for the test table I give the the SHA1 in the plotly filename so I hope the plotly server will respond back with a unique plotly every time. Is that a correct interpretation of the filename argument?

https://github.com/ropensci/plotly-test-table/blob/6a034a3a65e43c03e76072a75118390f998933ed/index.R#L179

@mkcor for the test table I assign arbitrary column labels by typing them into

https://github.com/ropensci/plotly-test-table/blob/gh-pages/code_commits.csv

so yes I was using "bug" to indicate a previous version and the "reviewed" to indicate the newer version. Is that OK or would you prefer that I use labels such as "new" and "old" ?

So anyways I corrected the test table and so I guess this branch is ready to merge. Can I please get a +1?

[mkcor]

I gave you a 👍 a long time ago because you wouldn't do the changes I would ask for so I thought I'll just do them myself...
Can you please remove files R/print.R, tests/testthat.R, and tests/testthat/test-ggplot-area.R from the changeset? They have nothing to do in this PR!

[tdhock]

Sure I can remove those files, if you tell me how?

[tdhock]

sorry I am a bit confused. should I just make the reverse changes and then commit them? or is there some other git command?

[tdhock]

And I'm sorry if I did not do all the changes you asked for --- I tried to address all of your comments.

[mkcor]

Well, I could have been more proactive myself and pulled the branch a while ago. Let me do it now and I'll share my recommendations.

[mkcor]

So, I couldn't revert commits, because these dummy changes occurred in the midst of other stuff, i.e., adcc2ba or 4766545 (hence my suspicion against git commit -a).

Actually I had pulled your branch before, but never pushed. So I did:

git checkout toby-fixes
git pull origin toby-fixes
git rm R/print.R git commit -m "Remove funny file"# Looked up id/hash for latest commit on master
git checkout 1d75115f719082 tests/testthat.R
git add tests/testthat.R git commit -m "Revert testthat.R file to latest commit on master"
git checkout 1d75115f719082 tests/testthat/test-ggplot-area.R
git add tests/testthat/test-ggplot-area.R git commit -m "Revert unrelated test file to latest commit on master"
git push origin toby-fixes

Hope you find this inspiring!

[mkcor]

+1

[tdhock]

Hey guys, I coded some fixes for geom_errorbar and geom_errorbarh, which is the first problem in #161

I changed the way traces are merged at the end of gg2list, so now it does not matter if the error bars come before or after the other geoms (points, lines).

I also moved the definition of a few constants from corresp_one_one.R to the top of ggplotly.R -- this saves me time during development and debugging since I only have to source("ggplotly.R") instead of both code files (in the correct order). I hope this is not a big deal for you guys.

Tests pass on my machine.

[mkcor]

Thanks, @tdhock -- Before I actually review...

moved the definition of a few constants from corresp_one_one.R to the top of ggplotly.R

Nooooooooooooooooo!!! We are trying to modularize this code base.

You shouldn't only source("ggplotly.R") as you develop, you need the entire package to test properly that your changes are working. Typically, you should use a Makefile like:

all: build
R --interactive < test_script.R
build:
R CMD build /dir/to/your/repo/plotly
R CMD INSTALL --library=/dir/to/your/wip/lib plotly_version#.tar.gz

where test_script.R contains, say, a new conversion you're working on, so at the end you can make sure that the whole

$ make

runs successfully. As you develop, you probably insert some browser() calls here and there. So then you should

$ make build

Make sure your .Rhistory is never saved. Make sure your WIP version of "plotly" gets installed at the right place (not the default library for R packages). Your test_script.R should typically start with

require(plotly, lib.loc="/dir/to/your/wip/lib")

Then launch R and source("test_script.R"). Finally, to check that the test suite runs well,

$ cd tests
$ R

and, in the R console,

require(plotly, lib.loc="/dir/to/your/wip/lib")
source("testthat.R")

Cool, now this is documented somewhere.

[tdhock]

ok sorry about that, I moved the constants back to the other file.

[tdhock]

I have also included a bunch of tests from https://github.com/chriddyp/ggplot2-plotly-cookbook/blob/master/axes.R#L70-L71 which I will eventually make pass.

[mkcor]

Test-driven development, yes!

[mkcor]

Remove trailing white line.

[mkcor]

I wish this PR was split into 10 PRs... Would make the review process faster and safer.

[tdhock]

The updated test table shows the most recent version of this branch as toby.fixes.reviewed

http://ropensci.github.io/plotly-test-table/tables/c47b8026a456a0ae382011db8ac4cce074fe5b1d/index.html

everything looks fine to me, ready to merge if you guys are.

[mkcor]

I'm happy with the changes so far! If I find something 'missing', I'll submit a PR for you to review @tdhock ;)
👍
/cc @chriddyp

[chriddyp]

@tdhock I noticed this ticks-flip was fixed but then not here:

[mkcor]

@chriddyp Good catch! But I think we can leave this fix for another/different/upcoming PR, since this one is huge already. (@tdhock Do you append '.bug' when running the image test suite on a feature branch, and then '.reviewed' when you re-run it on that same branch once the code has been reviewed?)

[mkcor]

I think I'll increment the middle digit in version number, after this PR is merged!
On the contrary, fixing ticks-flip will only increment the last digit.

[tdhock]

@chriddyp turns out my plotly code was correct for that flipped example,

http://ropensci.github.io/plotly-test-table/tables/c47b8026a456a0ae382011db8ac4cce074fe5b1d/ticks-flip.html

but the plotly-test-table code was not. In particular the plotly filename in the JSON kwargs was the same for different versions of the ggplotly code, so actually the non-flipped image that showed up was made using the code from Carson's branch, and the plotly server did not update that image file right away.

But now for the test table I give the the SHA1 in the plotly filename so I hope the plotly server will respond back with a unique plotly every time. Is that a correct interpretation of the filename argument?

https://github.com/ropensci/plotly-test-table/blob/6a034a3a65e43c03e76072a75118390f998933ed/index.R#L179

@mkcor for the test table I assign arbitrary column labels by typing them into

https://github.com/ropensci/plotly-test-table/blob/gh-pages/code_commits.csv

so yes I was using "bug" to indicate a previous version and the "reviewed" to indicate the newer version. Is that OK or would you prefer that I use labels such as "new" and "old" ?

So anyways I corrected the test table and so I guess this branch is ready to merge. Can I please get a +1?

[mkcor]

I gave you a 👍 a long time ago because you wouldn't do the changes I would ask for so I thought I'll just do them myself...
Can you please remove files R/print.R, tests/testthat.R, and tests/testthat/test-ggplot-area.R from the changeset? They have nothing to do in this PR!

[tdhock]

Sure I can remove those files, if you tell me how?

[tdhock]

sorry I am a bit confused. should I just make the reverse changes and then commit them? or is there some other git command?

[tdhock]

And I'm sorry if I did not do all the changes you asked for --- I tried to address all of your comments.

[mkcor]

Well, I could have been more proactive myself and pulled the branch a while ago. Let me do it now and I'll share my recommendations.

[mkcor]

So, I couldn't revert commits, because these dummy changes occurred in the midst of other stuff, i.e., adcc2ba or 4766545 (hence my suspicion against git commit -a).

Actually I had pulled your branch before, but never pushed. So I did:

git checkout toby-fixes
git pull origin toby-fixes
git rm R/print.R git commit -m "Remove funny file"# Looked up id/hash for latest commit on master
git checkout 1d75115f719082 tests/testthat.R
git add tests/testthat.R git commit -m "Revert testthat.R file to latest commit on master"
git checkout 1d75115f719082 tests/testthat/test-ggplot-area.R
git add tests/testthat/test-ggplot-area.R git commit -m "Revert unrelated test file to latest commit on master"
git push origin toby-fixes

Hope you find this inspiring!

[mkcor]

+1

[tdhock]

Hey guys, I coded some fixes for geom_errorbar and geom_errorbarh, which is the first problem in #161

I changed the way traces are merged at the end of gg2list, so now it does not matter if the error bars come before or after the other geoms (points, lines).

I also moved the definition of a few constants from corresp_one_one.R to the top of ggplotly.R -- this saves me time during development and debugging since I only have to source("ggplotly.R") instead of both code files (in the correct order). I hope this is not a big deal for you guys.

Tests pass on my machine.

[mkcor]

Thanks, @tdhock -- Before I actually review...

moved the definition of a few constants from corresp_one_one.R to the top of ggplotly.R

Nooooooooooooooooo!!! We are trying to modularize this code base.

You shouldn't only source("ggplotly.R") as you develop, you need the entire package to test properly that your changes are working. Typically, you should use a Makefile like:

all: build
R --interactive < test_script.R
build:
R CMD build /dir/to/your/repo/plotly
R CMD INSTALL --library=/dir/to/your/wip/lib plotly_version#.tar.gz

where test_script.R contains, say, a new conversion you're working on, so at the end you can make sure that the whole

$ make

runs successfully. As you develop, you probably insert some browser() calls here and there. So then you should

$ make build

Make sure your .Rhistory is never saved. Make sure your WIP version of "plotly" gets installed at the right place (not the default library for R packages). Your test_script.R should typically start with

require(plotly, lib.loc="/dir/to/your/wip/lib")

Then launch R and source("test_script.R"). Finally, to check that the test suite runs well,

$ cd tests
$ R

and, in the R console,

require(plotly, lib.loc="/dir/to/your/wip/lib")
source("testthat.R")

Cool, now this is documented somewhere.

[tdhock]

ok sorry about that, I moved the constants back to the other file.

[tdhock]

I have also included a bunch of tests from https://github.com/chriddyp/ggplot2-plotly-cookbook/blob/master/axes.R#L70-L71 which I will eventually make pass.

[mkcor]

Test-driven development, yes!

[mkcor]

Remove trailing white line.

[mkcor]

I wish this PR was split into 10 PRs... Would make the review process faster and safer.

[tdhock]

The updated test table shows the most recent version of this branch as toby.fixes.reviewed

http://ropensci.github.io/plotly-test-table/tables/c47b8026a456a0ae382011db8ac4cce074fe5b1d/index.html

everything looks fine to me, ready to merge if you guys are.

[mkcor]

I'm happy with the changes so far! If I find something 'missing', I'll submit a PR for you to review @tdhock ;)
👍
/cc @chriddyp

[chriddyp]

@tdhock I noticed this ticks-flip was fixed but then not here:

[mkcor]

@chriddyp Good catch! But I think we can leave this fix for another/different/upcoming PR, since this one is huge already. (@tdhock Do you append '.bug' when running the image test suite on a feature branch, and then '.reviewed' when you re-run it on that same branch once the code has been reviewed?)

[mkcor]

I think I'll increment the middle digit in version number, after this PR is merged!
On the contrary, fixing ticks-flip will only increment the last digit.

[tdhock]

@chriddyp turns out my plotly code was correct for that flipped example,

http://ropensci.github.io/plotly-test-table/tables/c47b8026a456a0ae382011db8ac4cce074fe5b1d/ticks-flip.html

but the plotly-test-table code was not. In particular the plotly filename in the JSON kwargs was the same for different versions of the ggplotly code, so actually the non-flipped image that showed up was made using the code from Carson's branch, and the plotly server did not update that image file right away.

But now for the test table I give the the SHA1 in the plotly filename so I hope the plotly server will respond back with a unique plotly every time. Is that a correct interpretation of the filename argument?

https://github.com/ropensci/plotly-test-table/blob/6a034a3a65e43c03e76072a75118390f998933ed/index.R#L179

@mkcor for the test table I assign arbitrary column labels by typing them into

https://github.com/ropensci/plotly-test-table/blob/gh-pages/code_commits.csv

so yes I was using "bug" to indicate a previous version and the "reviewed" to indicate the newer version. Is that OK or would you prefer that I use labels such as "new" and "old" ?

So anyways I corrected the test table and so I guess this branch is ready to merge. Can I please get a +1?

[mkcor]

I gave you a 👍 a long time ago because you wouldn't do the changes I would ask for so I thought I'll just do them myself...
Can you please remove files R/print.R, tests/testthat.R, and tests/testthat/test-ggplot-area.R from the changeset? They have nothing to do in this PR!

[tdhock]

Sure I can remove those files, if you tell me how?

[tdhock]

sorry I am a bit confused. should I just make the reverse changes and then commit them? or is there some other git command?

[tdhock]

And I'm sorry if I did not do all the changes you asked for --- I tried to address all of your comments.

[mkcor]

Well, I could have been more proactive myself and pulled the branch a while ago. Let me do it now and I'll share my recommendations.

[mkcor]

So, I couldn't revert commits, because these dummy changes occurred in the midst of other stuff, i.e., adcc2ba or 4766545 (hence my suspicion against git commit -a).

Actually I had pulled your branch before, but never pushed. So I did:

git checkout toby-fixes
git pull origin toby-fixes
git rm R/print.R git commit -m "Remove funny file"# Looked up id/hash for latest commit on master
git checkout 1d75115f719082 tests/testthat.R
git add tests/testthat.R git commit -m "Revert testthat.R file to latest commit on master"
git checkout 1d75115f719082 tests/testthat/test-ggplot-area.R
git add tests/testthat/test-ggplot-area.R git commit -m "Revert unrelated test file to latest commit on master"
git push origin toby-fixes

Hope you find this inspiring!

[mkcor]

+1

[tdhock]

Hey guys, I coded some fixes for geom_errorbar and geom_errorbarh, which is the first problem in #161

I changed the way traces are merged at the end of gg2list, so now it does not matter if the error bars come before or after the other geoms (points, lines).

I also moved the definition of a few constants from corresp_one_one.R to the top of ggplotly.R -- this saves me time during development and debugging since I only have to source("ggplotly.R") instead of both code files (in the correct order). I hope this is not a big deal for you guys.

Tests pass on my machine.

[mkcor]

Thanks, @tdhock -- Before I actually review...

moved the definition of a few constants from corresp_one_one.R to the top of ggplotly.R

Nooooooooooooooooo!!! We are trying to modularize this code base.

You shouldn't only source("ggplotly.R") as you develop, you need the entire package to test properly that your changes are working. Typically, you should use a Makefile like:

all: build
R --interactive < test_script.R
build:
R CMD build /dir/to/your/repo/plotly
R CMD INSTALL --library=/dir/to/your/wip/lib plotly_version#.tar.gz

where test_script.R contains, say, a new conversion you're working on, so at the end you can make sure that the whole

$ make

runs successfully. As you develop, you probably insert some browser() calls here and there. So then you should

$ make build

Make sure your .Rhistory is never saved. Make sure your WIP version of "plotly" gets installed at the right place (not the default library for R packages). Your test_script.R should typically start with

require(plotly, lib.loc="/dir/to/your/wip/lib")

Then launch R and source("test_script.R"). Finally, to check that the test suite runs well,

$ cd tests
$ R

and, in the R console,

require(plotly, lib.loc="/dir/to/your/wip/lib")
source("testthat.R")

Cool, now this is documented somewhere.

[tdhock]

ok sorry about that, I moved the constants back to the other file.

[tdhock]

I have also included a bunch of tests from https://github.com/chriddyp/ggplot2-plotly-cookbook/blob/master/axes.R#L70-L71 which I will eventually make pass.

[mkcor]

Test-driven development, yes!

[mkcor]

Remove trailing white line.

[mkcor]

I wish this PR was split into 10 PRs... Would make the review process faster and safer.

[tdhock]

The updated test table shows the most recent version of this branch as toby.fixes.reviewed

http://ropensci.github.io/plotly-test-table/tables/c47b8026a456a0ae382011db8ac4cce074fe5b1d/index.html

everything looks fine to me, ready to merge if you guys are.

[mkcor]

I'm happy with the changes so far! If I find something 'missing', I'll submit a PR for you to review @tdhock ;)
👍
/cc @chriddyp

[chriddyp]

@tdhock I noticed this ticks-flip was fixed but then not here:

[mkcor]

@chriddyp Good catch! But I think we can leave this fix for another/different/upcoming PR, since this one is huge already. (@tdhock Do you append '.bug' when running the image test suite on a feature branch, and then '.reviewed' when you re-run it on that same branch once the code has been reviewed?)

[mkcor]

I think I'll increment the middle digit in version number, after this PR is merged!
On the contrary, fixing ticks-flip will only increment the last digit.

[tdhock]

@chriddyp turns out my plotly code was correct for that flipped example,

http://ropensci.github.io/plotly-test-table/tables/c47b8026a456a0ae382011db8ac4cce074fe5b1d/ticks-flip.html

but the plotly-test-table code was not. In particular the plotly filename in the JSON kwargs was the same for different versions of the ggplotly code, so actually the non-flipped image that showed up was made using the code from Carson's branch, and the plotly server did not update that image file right away.

But now for the test table I give the the SHA1 in the plotly filename so I hope the plotly server will respond back with a unique plotly every time. Is that a correct interpretation of the filename argument?

https://github.com/ropensci/plotly-test-table/blob/6a034a3a65e43c03e76072a75118390f998933ed/index.R#L179

@mkcor for the test table I assign arbitrary column labels by typing them into

https://github.com/ropensci/plotly-test-table/blob/gh-pages/code_commits.csv

so yes I was using "bug" to indicate a previous version and the "reviewed" to indicate the newer version. Is that OK or would you prefer that I use labels such as "new" and "old" ?

So anyways I corrected the test table and so I guess this branch is ready to merge. Can I please get a +1?

[mkcor]

I gave you a 👍 a long time ago because you wouldn't do the changes I would ask for so I thought I'll just do them myself...
Can you please remove files R/print.R, tests/testthat.R, and tests/testthat/test-ggplot-area.R from the changeset? They have nothing to do in this PR!

[tdhock]

Sure I can remove those files, if you tell me how?

[tdhock]

sorry I am a bit confused. should I just make the reverse changes and then commit them? or is there some other git command?

[tdhock]

And I'm sorry if I did not do all the changes you asked for --- I tried to address all of your comments.

[mkcor]

Well, I could have been more proactive myself and pulled the branch a while ago. Let me do it now and I'll share my recommendations.

[mkcor]

So, I couldn't revert commits, because these dummy changes occurred in the midst of other stuff, i.e., adcc2ba or 4766545 (hence my suspicion against git commit -a).

Actually I had pulled your branch before, but never pushed. So I did:

git checkout toby-fixes
git pull origin toby-fixes
git rm R/print.R git commit -m "Remove funny file"# Looked up id/hash for latest commit on master
git checkout 1d75115f719082 tests/testthat.R
git add tests/testthat.R git commit -m "Revert testthat.R file to latest commit on master"
git checkout 1d75115f719082 tests/testthat/test-ggplot-area.R
git add tests/testthat/test-ggplot-area.R git commit -m "Revert unrelated test file to latest commit on master"
git push origin toby-fixes

Hope you find this inspiring!

[mkcor]

+1

[tdhock]

Hey guys, I coded some fixes for geom_errorbar and geom_errorbarh, which is the first problem in #161

I changed the way traces are merged at the end of gg2list, so now it does not matter if the error bars come before or after the other geoms (points, lines).

I also moved the definition of a few constants from corresp_one_one.R to the top of ggplotly.R -- this saves me time during development and debugging since I only have to source("ggplotly.R") instead of both code files (in the correct order). I hope this is not a big deal for you guys.

Tests pass on my machine.

[mkcor]

Thanks, @tdhock -- Before I actually review...

moved the definition of a few constants from corresp_one_one.R to the top of ggplotly.R

Nooooooooooooooooo!!! We are trying to modularize this code base.

You shouldn't only source("ggplotly.R") as you develop, you need the entire package to test properly that your changes are working. Typically, you should use a Makefile like:

all: build
R --interactive < test_script.R
build:
R CMD build /dir/to/your/repo/plotly
R CMD INSTALL --library=/dir/to/your/wip/lib plotly_version#.tar.gz

where test_script.R contains, say, a new conversion you're working on, so at the end you can make sure that the whole

$ make

runs successfully. As you develop, you probably insert some browser() calls here and there. So then you should

$ make build

Make sure your .Rhistory is never saved. Make sure your WIP version of "plotly" gets installed at the right place (not the default library for R packages). Your test_script.R should typically start with

require(plotly, lib.loc="/dir/to/your/wip/lib")

Then launch R and source("test_script.R"). Finally, to check that the test suite runs well,

$ cd tests
$ R

and, in the R console,

require(plotly, lib.loc="/dir/to/your/wip/lib")
source("testthat.R")

Cool, now this is documented somewhere.

[tdhock]

ok sorry about that, I moved the constants back to the other file.

[tdhock]

I have also included a bunch of tests from https://github.com/chriddyp/ggplot2-plotly-cookbook/blob/master/axes.R#L70-L71 which I will eventually make pass.

[mkcor]

Test-driven development, yes!

[mkcor]

Remove trailing white line.

[mkcor]

I wish this PR was split into 10 PRs... Would make the review process faster and safer.

[tdhock]

The updated test table shows the most recent version of this branch as toby.fixes.reviewed

http://ropensci.github.io/plotly-test-table/tables/c47b8026a456a0ae382011db8ac4cce074fe5b1d/index.html

everything looks fine to me, ready to merge if you guys are.

[mkcor]

I'm happy with the changes so far! If I find something 'missing', I'll submit a PR for you to review @tdhock ;)
👍
/cc @chriddyp

[chriddyp]

@tdhock I noticed this ticks-flip was fixed but then not here:

[mkcor]

@chriddyp Good catch! But I think we can leave this fix for another/different/upcoming PR, since this one is huge already. (@tdhock Do you append '.bug' when running the image test suite on a feature branch, and then '.reviewed' when you re-run it on that same branch once the code has been reviewed?)

[mkcor]

I think I'll increment the middle digit in version number, after this PR is merged!
On the contrary, fixing ticks-flip will only increment the last digit.

[tdhock]

@chriddyp turns out my plotly code was correct for that flipped example,

http://ropensci.github.io/plotly-test-table/tables/c47b8026a456a0ae382011db8ac4cce074fe5b1d/ticks-flip.html

but the plotly-test-table code was not. In particular the plotly filename in the JSON kwargs was the same for different versions of the ggplotly code, so actually the non-flipped image that showed up was made using the code from Carson's branch, and the plotly server did not update that image file right away.

But now for the test table I give the the SHA1 in the plotly filename so I hope the plotly server will respond back with a unique plotly every time. Is that a correct interpretation of the filename argument?

https://github.com/ropensci/plotly-test-table/blob/6a034a3a65e43c03e76072a75118390f998933ed/index.R#L179

@mkcor for the test table I assign arbitrary column labels by typing them into

https://github.com/ropensci/plotly-test-table/blob/gh-pages/code_commits.csv

so yes I was using "bug" to indicate a previous version and the "reviewed" to indicate the newer version. Is that OK or would you prefer that I use labels such as "new" and "old" ?

So anyways I corrected the test table and so I guess this branch is ready to merge. Can I please get a +1?

[mkcor]

I gave you a 👍 a long time ago because you wouldn't do the changes I would ask for so I thought I'll just do them myself...
Can you please remove files R/print.R, tests/testthat.R, and tests/testthat/test-ggplot-area.R from the changeset? They have nothing to do in this PR!

[tdhock]

Sure I can remove those files, if you tell me how?

[tdhock]

sorry I am a bit confused. should I just make the reverse changes and then commit them? or is there some other git command?

[tdhock]

And I'm sorry if I did not do all the changes you asked for --- I tried to address all of your comments.

[mkcor]

Well, I could have been more proactive myself and pulled the branch a while ago. Let me do it now and I'll share my recommendations.

[mkcor]

So, I couldn't revert commits, because these dummy changes occurred in the midst of other stuff, i.e., adcc2ba or 4766545 (hence my suspicion against git commit -a).

Actually I had pulled your branch before, but never pushed. So I did:

git checkout toby-fixes
git pull origin toby-fixes
git rm R/print.R git commit -m "Remove funny file"# Looked up id/hash for latest commit on master
git checkout 1d75115f719082 tests/testthat.R
git add tests/testthat.R git commit -m "Revert testthat.R file to latest commit on master"
git checkout 1d75115f719082 tests/testthat/test-ggplot-area.R
git add tests/testthat/test-ggplot-area.R git commit -m "Revert unrelated test file to latest commit on master"
git push origin toby-fixes

Hope you find this inspiring!

[mkcor]

+1

[tdhock]

Hey guys, I coded some fixes for geom_errorbar and geom_errorbarh, which is the first problem in #161

I changed the way traces are merged at the end of gg2list, so now it does not matter if the error bars come before or after the other geoms (points, lines).

I also moved the definition of a few constants from corresp_one_one.R to the top of ggplotly.R -- this saves me time during development and debugging since I only have to source("ggplotly.R") instead of both code files (in the correct order). I hope this is not a big deal for you guys.

Tests pass on my machine.

[mkcor]

Thanks, @tdhock -- Before I actually review...

moved the definition of a few constants from corresp_one_one.R to the top of ggplotly.R

Nooooooooooooooooo!!! We are trying to modularize this code base.

You shouldn't only source("ggplotly.R") as you develop, you need the entire package to test properly that your changes are working. Typically, you should use a Makefile like:

all: build
R --interactive < test_script.R
build:
R CMD build /dir/to/your/repo/plotly
R CMD INSTALL --library=/dir/to/your/wip/lib plotly_version#.tar.gz

where test_script.R contains, say, a new conversion you're working on, so at the end you can make sure that the whole

$ make

runs successfully. As you develop, you probably insert some browser() calls here and there. So then you should

$ make build

Make sure your .Rhistory is never saved. Make sure your WIP version of "plotly" gets installed at the right place (not the default library for R packages). Your test_script.R should typically start with

require(plotly, lib.loc="/dir/to/your/wip/lib")

Then launch R and source("test_script.R"). Finally, to check that the test suite runs well,

$ cd tests
$ R

and, in the R console,

require(plotly, lib.loc="/dir/to/your/wip/lib")
source("testthat.R")

Cool, now this is documented somewhere.

[tdhock]

ok sorry about that, I moved the constants back to the other file.

[tdhock]

I have also included a bunch of tests from https://github.com/chriddyp/ggplot2-plotly-cookbook/blob/master/axes.R#L70-L71 which I will eventually make pass.

[mkcor]

Test-driven development, yes!

[mkcor]

Remove trailing white line.

[mkcor]

I wish this PR was split into 10 PRs... Would make the review process faster and safer.

[tdhock]

The updated test table shows the most recent version of this branch as toby.fixes.reviewed

http://ropensci.github.io/plotly-test-table/tables/c47b8026a456a0ae382011db8ac4cce074fe5b1d/index.html

everything looks fine to me, ready to merge if you guys are.

[mkcor]

I'm happy with the changes so far! If I find something 'missing', I'll submit a PR for you to review @tdhock ;)
👍
/cc @chriddyp

[chriddyp]

@tdhock I noticed this ticks-flip was fixed but then not here:

[mkcor]

@chriddyp Good catch! But I think we can leave this fix for another/different/upcoming PR, since this one is huge already. (@tdhock Do you append '.bug' when running the image test suite on a feature branch, and then '.reviewed' when you re-run it on that same branch once the code has been reviewed?)

[mkcor]

I think I'll increment the middle digit in version number, after this PR is merged!
On the contrary, fixing ticks-flip will only increment the last digit.

[tdhock]

@chriddyp turns out my plotly code was correct for that flipped example,

http://ropensci.github.io/plotly-test-table/tables/c47b8026a456a0ae382011db8ac4cce074fe5b1d/ticks-flip.html

but the plotly-test-table code was not. In particular the plotly filename in the JSON kwargs was the same for different versions of the ggplotly code, so actually the non-flipped image that showed up was made using the code from Carson's branch, and the plotly server did not update that image file right away.

But now for the test table I give the the SHA1 in the plotly filename so I hope the plotly server will respond back with a unique plotly every time. Is that a correct interpretation of the filename argument?

https://github.com/ropensci/plotly-test-table/blob/6a034a3a65e43c03e76072a75118390f998933ed/index.R#L179

@mkcor for the test table I assign arbitrary column labels by typing them into

https://github.com/ropensci/plotly-test-table/blob/gh-pages/code_commits.csv

so yes I was using "bug" to indicate a previous version and the "reviewed" to indicate the newer version. Is that OK or would you prefer that I use labels such as "new" and "old" ?

So anyways I corrected the test table and so I guess this branch is ready to merge. Can I please get a +1?

[mkcor]

I gave you a 👍 a long time ago because you wouldn't do the changes I would ask for so I thought I'll just do them myself...
Can you please remove files R/print.R, tests/testthat.R, and tests/testthat/test-ggplot-area.R from the changeset? They have nothing to do in this PR!

[tdhock]

Sure I can remove those files, if you tell me how?

[tdhock]

sorry I am a bit confused. should I just make the reverse changes and then commit them? or is there some other git command?

[tdhock]

And I'm sorry if I did not do all the changes you asked for --- I tried to address all of your comments.

[mkcor]

Well, I could have been more proactive myself and pulled the branch a while ago. Let me do it now and I'll share my recommendations.

[mkcor]

So, I couldn't revert commits, because these dummy changes occurred in the midst of other stuff, i.e., adcc2ba or 4766545 (hence my suspicion against git commit -a).

Actually I had pulled your branch before, but never pushed. So I did:

git checkout toby-fixes
git pull origin toby-fixes
git rm R/print.R git commit -m "Remove funny file"# Looked up id/hash for latest commit on master
git checkout 1d75115f719082 tests/testthat.R
git add tests/testthat.R git commit -m "Revert testthat.R file to latest commit on master"
git checkout 1d75115f719082 tests/testthat/test-ggplot-area.R
git add tests/testthat/test-ggplot-area.R git commit -m "Revert unrelated test file to latest commit on master"
git push origin toby-fixes

Hope you find this inspiring!

[mkcor]

+1