Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

citation & doi formatting #282

Merged
merged 9 commits into from
Jul 5, 2024
Merged

citation & doi formatting #282

merged 9 commits into from
Jul 5, 2024

Conversation

zeileis
Copy link
Contributor

@zeileis zeileis commented Jul 4, 2024

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

In rd2qmd.R:

  • \doi{...} tags in Rd pages are now processed so that they show up as hyperlinks. Rd2HTML() expands these to \Sexpr[results=rd]{tools:::Rd_expr_doi("...")} which subsequently gets ignored by quarto. Hence I have added a regexp to replace this with a markdown link.
  • The library() call added in the examples now avoids non-standard evaluation.

In import_misc.R:

  • The citation file generated from citation(...) now uses Markdown/HTML rather than verbatim code formatting. This is done by using the format() method for citation objects (plus separate processing for the mheader/header of the citation, if any).

@etiennebacher
Copy link
Owner

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
Copy link
Contributor Author

zeileis commented Jul 4, 2024

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
Copy link
Collaborator

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

@zeileis
Copy link
Contributor Author

zeileis commented Jul 4, 2024

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
Copy link
Contributor Author

zeileis commented Jul 4, 2024

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 🤓

@etiennebacher
Copy link
Owner

etiennebacher commented Jul 4, 2024

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
Copy link
Collaborator

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

Copy link
Owner

@etiennebacher etiennebacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just changed formatting a bit but looks good otherwise, thanks @zeileis !

@etiennebacher etiennebacher merged commit 6116ed5 into etiennebacher:main Jul 5, 2024
5 checks passed
@zeileis
Copy link
Contributor Author

zeileis commented Jul 5, 2024

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
Copy link
Owner

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
Copy link
Contributor Author

zeileis commented Jul 5, 2024

Fair enough!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants