Feedback on the package

[karthik]

Hi @taddallas

Here is some feedback/questions on the package.

Did you write all the documentation by hand? It would be worth converting to/learning how to use roxygen2 tags. This would allow you to write documentation inline and keep it updated. It would also take care of your NAMESPACE without any manual work.

There are no examples on any of your functions inline. These would also get taken care of by the roxygen documentation.

There are no tests (so Travis is not doing much to be useful).

You committed a temp file (README.md~). You'll need to delete that.

Have you considered writing a vignette? It would provide more context to why someone would use this package and what sorts of applications exist.

[taddallas]

Hi @karthik

Thanks for your feedback! I had a couple of questions.

First, what do you mean by examples of functions inline? Inline in the man files?

Second, what sorts of tests should Travis be conducting? I naively thought that Travis was just checking to make sure the package could be loaded and examples could be run (even though this is redundant with me locally R CMD check/build-ing at every change).

I did write all the documentation by hand, but I don't see the draw of using roxygen2. Maybe I'm just missing something. It seems nice that you can include documentation alongside of code, and the man files are made on-the-fly. However, it'll probably take more than that to convince me to delete what I already have written or to copy all that stuff over to the R files with appropriate syntax. I'll look into writing a vignette. I figured the package was too bare bones to really warrant a vignette (it really only has 3 functions of note). Perhaps I'll work on adding a plot type function or a data manipulation function before the vignette. Thoughts?

[karthik]

First, what do you mean by examples of functions inline? Inline in the man files?

Most of the rOpenSci workflow these days relies on devtools since it does provide more automated checks and eases development pain points. As a R developer, I mostly read function documentation and examples inline, rarely from help files (unless running a package that I haven't developed and am using from the CL). It also allows for faster fixes to examples and docs when making changes since you are in the same document. Making updates in two separate locations manually provides more room for errors.

I did write all the documentation by hand, but I don't see the draw of using roxygen2. Maybe I'm just missing something. It seems nice that you can include documentation alongside of code, and the man files are made on-the-fly.

Yes exactly, that's what is great about it. It also takes care of namespaces, methods and such without additional manual checks.

However, it'll probably take more than that to convince me to delete what I already have written or to copy all that stuff over to the R files with appropriate syntax.

No need to do it manually. There is a way to take existing docs and convert them back to roxygen tags. @sckott and I can help you there.

Second, what sorts of tests should Travis be conducting? I naively thought that Travis was just checking to make sure the package could be loaded and examples could be run (even though this is redundant with me locally R CMD check/build-ing at every change).

Travis runs examples but you should also explicitly have tests to make sure your functions do what they say on the tin. The package testthat makes it easy to write tests. See some notes I wrote here: https://github.com/karthik/dlab-advanced-r/tree/master/02-testing

With devtools, you can simply run: use_testthat and it will add the appropriate files. You can see some extensive tests that @sckott has written for various packages under the rOpenSci account. Making changes and committing them to GitHub doesn't require you to run checks. So there will be situations in the future where you might fix a fn, then push and not realize it broke check. Travis will run check on each commit on a separate box temporarily created for just this purpose. It is a great, automated way to ensure CI and test driven development.

I agree that it might be too early for a vignette. But it's generally not hard to start one and keep fleshing it out before waiting till the package becomes bigger. Again with devtools it is also easy to add with: use_vignette

@sckott did I miss anything else?

[taddallas]

Okay. I'll drink the Kool-aid.

I do see the utility in setting up tests, and have added some preliminary function tests. I also made some small function tweaks that format location values correctly and throw errors when you try to hand it a non-existent location. I'll add a plot function and a quick data manipulation function (form interaction matrix from edgelist output) sometime soon-ish.

I think what I'll need your help with the most is the conversion of my current documentation setup to the roxygen setup. Also, please continue to point out things I should be doing (writing tests, etc.).

[karthik]

Cool, I sent a PR.

[taddallas]

@karthik I'm closing the issue, as I think we covered much of this ground. I really appreciate all your help! Don't hesitate to point out any other potential improvements.