etiennebacher / altdoc

Alternative to pkgdown to document R packages
https://altdoc.etiennebacher.com
Other
62 stars 9 forks source link

citation & doi formatting #282

Closed zeileis closed 1 month ago

zeileis commented 1 month ago

Etienne @etiennebacher & Vincent @vincentarelbundock, in this PR I propose a few small improvements:

In rd2qmd.R:

In import_misc.R:

etiennebacher commented 1 month ago

Thanks @zeileis, those seem like a good idea, I'll try to take a look this weekend.

Can you add tests for those changes? There is a dummy test package in https://github.com/etiennebacher/altdoc/tree/main/tests/testthat/examples/testpkg.altdoc where you can for instance add a \doi{} tag in one of the Rd files and then update the snapshots by running testthat.

The library() call added in the examples now avoids non-standard evaluation.

Why? Is this discouraged? I personally always use library(foo) instead of library("foo")

zeileis commented 1 month ago

Re: library. This is really confusing when teaching beginners. First, you tell them that print(x) is fundamentally different from print("x"). Then half an hour later you need to explain why library(openxlsx) is not different from library("openxlsx"). Additionally, the syntax highlighting is better when avoiding NSE.

But if you (or some of your users) feel strongly about using the NSE version, it would be nice to make it optional.

vincentarelbundock commented 1 month ago

I should start using the quoted versions all the time. FWIW, I agree with AZ on this.

zeileis commented 1 month ago

Re: tests. I have added now both a @references section with \doi{...} and a @seealso section (just in case we want to test hyperlinking for this) and re-ran devtools::document().

I also tried to update the snapshots by running testthat but I think I didn't do it correctly because more things changed than just what I added. So I haven't committed this changes. (Sorry if I'm too dumb here, I haven't got much experience with testthat...)

zeileis commented 1 month ago

BTW: If you want to see an example for the changes in the wild, check out https://betareg.R-Forge.R-project.org. I'm currently working on a new betareg release (because I'm presenting it next week at useR!) and hence put together the altdoc quarto page :nerd_face:

etiennebacher commented 1 month ago

Re: library. This is really confusing when teaching beginners

I see, I didn't teach to beginners yet so didn't have this experience. I don't have strong preferences, we can keep library("foo").

I did a mistake in a previous PR where I modified directly the Rd file instead of using roxygen2 in the test package folder. I'll fix this and regenerate the snapshots. FYI running devtools::test() should provoke failures because snapshots have changed, and then you can run testthat::snapshot_accept() to keep the new snapshots. It's easier to see the differences with git than in the R console.

I wish I was at UseR! to meet both of you, maybe next time ;)

vincentarelbundock commented 1 month ago

FYI, I just updated the PR with some minor changes to main.

zeileis commented 1 month ago

Great, thanks a lot for considering this! And thanks also for the testthat explanations. I will try it out the next time.

Re: formatting. Indexing with 1L rather than 1 is not just about formatting but avoiding the internal coercion from numeric 1 to integer 1L. The difference is tiny and here surely not relevant. But I started using explicit integers for all subset in code...

etiennebacher commented 1 month ago

Re: formatting. Indexing with 1L rather than 1 is not just about formatting but avoiding the internal coercion from numeric 1 to integer 1L.

I know but I find this less readable. I understand why one would want to avoid coercion in a package built to be super performant like data.table for instance, but here I don't think it matters so I prefer having less cluttered code ;)

zeileis commented 1 month ago

Fair enough!