yihui / knitr

A general-purpose tool for dynamic report generation in R
https://yihui.org/knitr/
2.36k stars 873 forks source link

write_bib() includes multiple URLs #2343

Closed bastistician closed 1 month ago

bastistician commented 2 months ago

The intention of #2264 was "if multiple URLs are present, drop all but the first" and that is also what is documented in help(write_bib):

the first URL listed in the ‘DESCRIPTION’ file will be used.

Unfortunately, this is not always true and may depend on the R version. For example:

packageDescription("polyclip", fields = "URL")
#> [1] "http://www.angusj.com/delphi/clipper.php,\nhttps://sourceforge.net/projects/polyclipping,\nhttps://github.com/baddstats/polyclip"
knitr::write_bib("polyclip")

In R 4.3.0 (as intended):

@Manual{R-polyclip,
  title = {polyclip: Polygon Clipping},
  author = {Angus Johnson and Adrian Baddeley},
  year = {2023},
  note = {R package version 1.10-6},
  url = {http://www.angusj.com/delphi/clipper.php},
}

In R 4.4.0 (2nd URL in note field, 3rd URL ignored):

@Manual{R-polyclip,
  title = {polyclip: Polygon Clipping},
  author = {Angus Johnson and Adrian Baddeley},
  year = {2023},
  note = {R package version 1.10-6, 
https://sourceforge.net/projects/polyclipping},
  url = {http://www.angusj.com/delphi/clipper.php},
}

The problematic code seems to be here: https://github.com/yihui/knitr/blob/726fe1553ace340789e2a649b328c3ecb5ebae4a/R/citation.R#L96-L99 I don't understand where these two replacement steps came from, but the second one is only active if it finds a space (so not for "polyclip", see above). AFAICS, these four lines could simply be replaced by

meta$URL = sub('[, \t\n].*', '', meta$URL) 

given that the URLs can be separated by comma or whitespace.

yihui commented 1 month ago

AFAICS, these four lines could simply be replaced by

meta$URL = sub('[, \t\n].*', '', meta$URL) 

given that the URLs can be separated by comma or whitespace.

That sounds okay to me, but I'll let @dmurdoch confirm it since the two sub()s were from the PR #2264.

dmurdoch commented 1 month ago

I don't remember why I had the R < 4.3.2 dependency, but if it was necessary, at least it will go away over time.

yihui commented 1 month ago

Okay, then I guess we can try to use the single sub() as @bastistician suggested.