Remove C code #47

dieghernan · 2022-01-14T11:03:49Z

Hi:

Following #46 and related to #45 , this PR removes the C code of the package, substituting it by pure R (base) code. I am aware this is a critical change to the package, so let me share some points of attention on this PR:

Version bump to make clear that this is a dev version
trimws has been updated to the latest version of the same function on R, see https://github.com/wch/r-source/blob/5ab79ec84040684c74dc9c901fde944fff6e8375/src/library/base/R/strwrap.R#L219-L229

Some parameters have been deprecated:

src in do_read_bib() is not needed any more

read.bib(): header and footer. They were never actually used, see:

bibtex/R/bibtex.R

Lines 162 to 170 in 05a401c

    
           make.citation.list <- function( x, header, footer){ 
        
               rval <- list() 
        
               for( i in seq_along(x) ){ 
        
                   if( !is.null(x[[i]] ) ) 
        
                       rval <- c( rval, x[[i]] ) 
        
               } 
        
               class(rval) <- c( "bibentry" ) 
        
               rval 
        
           }

A whole set of functions have been added to utils.R. This is actually the replacement of C in R code.
An url on README has been updated following the urlcheck package.
There is a change on a snapshot. Actually now is correct, before was ommiting a part on the string:

@String{Rnews = "http://CRAN.R-project.org/doc/Rnews/" }
@String{Rnews2001-1 = rnews # "Rnews_2001-1.pdf"}
@String{Rnews2001-2 = rnews # "Rnews_2001-2.pdf"}
@String{Rnews2001-3 = rnews # "Rnews_2001-3.pdf"}

So Rnews2001-3 should be parsed as rnews # "Rnews_2001-3.pdf (http://CRAN.R-project.org/doc/Rnews/Rnews_2001-3.pdf)

I run revdepcheck::revdep_check() succesfully. This is specially important for maintainers of the affected packages (@renozao, @sckott, @njahn82, @bwiernik, @mwmclean )

Comments welcome

This would also close #42 , fix #16

#45

As defined in BibTEX version 0.99b https://ctan.javinator9889.com/biblio/bibtex/base/btxdoc.pdf

Possibly a character problem related with #20 and #43

Some changes in default parsing (snapshot), but results are still ok

Also bump version

Merge branch 'master' into phase2 # Conflicts: # tests/testthat/_snaps/examples.md # tests/testthat/test-fatal-errors.R

dieghernan · 2022-01-14T11:11:00Z

DESCRIPTION

@@ -1,5 +1,5 @@
 Package: bibtex
-Version: 0.4.3
+Version: 0.4.3.9000


Bump, this can be undone

Let's go 0.5.0 for the next CRAN release and, then, bump to 1.0.0.

dieghernan · 2022-01-14T11:12:15Z

R/bibtex.R

-trimws <- function(x) {
-  sub("^[[:space:]]+", "", sub("[[:space:]]$", "", x))
+# See https://github.com/wch/r-source/blob/trunk/src/library/base/R/strwrap.R
+trimws <- function(x, which = c("both", "left", "right"),


Updated version of the backport

Should we consider a dependency on the {backports} package to automatically get up-to-speed? Though, as it stands now, we only have a dependency on one base package: utils.

That said, we probably should bump the minimum version to at least R 3.6.* as we are not really testing if it works on R 3.0. Something to ponder for later on.

Good idea, I just added the backports, but agree on bumping the minimal R version as well

dieghernan · 2022-01-14T11:12:30Z

R/bibtex.R

@@ -196,10 +207,93 @@ findBibFile <- function(package) {
 #'
 #' @param file file name
 #' @param encoding encoding
-#' @param srcfile output of \code{\link{srcfile}}
+#' @param srcfile Deprecated


Not needed nor used any more

We should probably also modify the function signature to use '' and write a quick diagnostic message:

if (!missing("srcfile")) message("`srcfile` argument is deprecated.")

Done! I also added a couple of tests on this

dieghernan · 2022-01-14T11:12:51Z

R/bibtex.R

-#'        the bib file.
-#' @param footer footer of the citation list
+#' @param header DEPRECATED.
+#' @param footer DEPRECATED


Those were never used

See my previous note on deprecating arguments.

If a previous argument was never used, perhaps we could also just exclude these arguments altogether in the next version?

Done. I would favor removing those on the next major version (1.0.0) just in case. For v0.5.0 I prefer to leave them, so we can get feedback

dieghernan · 2022-01-14T11:14:29Z

README.md

@@ -38,4 +38,4 @@ donate it to rOpenSci.
 - Get citation information for `bibtex` in R with `citation("bibtex")`
 - Please note that this package is released with a [Contributor Code of Conduct](https://ropensci.org/code-of-conduct/). By contributing to this project, you agree to abide by its terms.

-[![ropensci](http://ropensci.org/public_images/github_footer.png)](http://ropensci.org)
+[![ropensci](https://ropensci.org//public_images/github_footer.png)](https://ropensci.org/)


Just an update of the url, see urlchecker::url_update()

Thanks for running the {urlchecker}!

dieghernan · 2022-01-14T11:17:32Z

tests/testthat/_snaps/examples.md

@@ -281,7 +281,7 @@
      pages = {20--23},
      month = {September},
      url = {http://CRAN.R-project.org/doc/Rnews/},
-      pdf = {rnewsRnews_2001-3.pdf},
+      pdf = {http://CRAN.R-project.org/doc/Rnews/Rnews_2001-3.pdf},


This is now correct, before it just parsed the last part of the String, see https://ctan.javinator9889.com/biblio/bibtex/base/btxdoc.pdf:

Thanks for catching the update!

dieghernan · 2022-01-14T11:17:52Z

tests/testthat/test-fatal-errors.R

@@ -1,12 +1,10 @@
 test_that("Unbalanced braces", {


Now we can test fatal errors safely, R does not crash any more

dieghernan · 2022-01-14T11:18:57Z

I added some comments on the review tab to make easier to understand some things @coatless

coatless

Impressive work with the regular expressions! Reading through the regex, I'm not sure we'll unintentionally introduce another problem; but, the users will surely let us know.

Outside of that, there are minor cleanups likely needed for long-term maintainability.

coatless · 2022-01-15T21:21:15Z

R/bibtex.R

-trimws <- function(x) {
-  sub("^[[:space:]]+", "", sub("[[:space:]]$", "", x))
+# See https://github.com/wch/r-source/blob/trunk/src/library/base/R/strwrap.R
+trimws <- function(x, which = c("both", "left", "right"),


Should we consider a dependency on the {backports} package to automatically get up-to-speed? Though, as it stands now, we only have a dependency on one base package: utils.

That said, we probably should bump the minimum version to at least R 3.6.* as we are not really testing if it works on R 3.0. Something to ponder for later on.

coatless · 2022-01-15T21:23:59Z

R/bibtex.R

@@ -196,10 +207,93 @@ findBibFile <- function(package) {
 #'
 #' @param file file name
 #' @param encoding encoding
-#' @param srcfile output of \code{\link{srcfile}}
+#' @param srcfile Deprecated


We should probably also modify the function signature to use '' and write a quick diagnostic message:

if (!missing("srcfile")) message("`srcfile` argument is deprecated.")

coatless · 2022-01-15T21:25:07Z

R/bibtex.R

+        NULL
+      },
+      error = function(e) {
+        stop("Error when parsing strings of ",


Is there a way we could indicate which line was problematic? Anything to make the error message more informative would be ideal here.

I think we are already doing that, see this reprex. Do you wish to improve it in any way?

# Message on parsing entries f <- file.path(system.file("bib", "badFormat.bib", package = "bibtex")) bib <- read.bib(f) #> ignoring entry 'brown:1963' (line 6) because : #> A bibentry of bibtype 'Book' has to specify the field: title #> ignoring entry 'chambers+cleveland+kleiner+tukey:1983' (line 21) because : #> Invalid author/editor format. # On unbalanced braces we throw an error indicating the problematic line f2 <- system.file("bib", "unbalanced_braces.bib", package = "bibtex") bib2 <- read.bib(f2) #> Error: Unbalanced braces on entry (line 9). Invalid .bib file #> Error: Invalid bib file file <- f2

See

bibtex/inst/bib/badFormat.bib

Lines 6 to 10 in 05a401c

@book{brown:1963,

author = "Brown, R. G.",

year = 1963,

publisher = "Prentice-Hall"

}

and

bibtex/inst/bib/unbalanced_braces.bib

Lines 9 to 14 in 05a401c

@misc{murdoch:2010,

author = "Duncan Murdoch",

title = {"Parsing {Rd} files unbalanced",

year = 2010,

url = "http://developer.r-project.org/parseRd.pdf"

}

coatless · 2022-01-15T21:26:12Z

R/bibtex.R

-#'        the bib file.
-#' @param footer footer of the citation list
+#' @param header DEPRECATED.
+#' @param footer DEPRECATED


See my previous note on deprecating arguments.

If a previous argument was never used, perhaps we could also just exclude these arguments altogether in the next version?

coatless · 2022-01-15T21:28:03Z

R/utils.R

+#'
+#' Returns a table with all the string values and their value to be used on the
+#' `bib` file.
+#'


Could we add an @param describing input? This is for long-term maintenance benefits as the function is internal.

I added documentation of params and also some examples on each internal function, hopefully that would help maintaining them

I added the examples also as tests on test-utils.R

coatless · 2022-01-15T21:35:20Z

R/utils.R

+#' Parse a single entry
+#'
+#' Returns an entry from a .bib file parsed into an R object
+#'


Please add some documentation on parameters. (Even though it's an internal function.)

Same, this is now documented and with examples

And the examples are also part of the tests

coatless · 2022-01-15T21:36:34Z

tests/testthat/_snaps/examples.md

@@ -281,7 +281,7 @@
      pages = {20--23},
      month = {September},
      url = {http://CRAN.R-project.org/doc/Rnews/},
-      pdf = {rnewsRnews_2001-3.pdf},
+      pdf = {http://CRAN.R-project.org/doc/Rnews/Rnews_2001-3.pdf},


Thanks for catching the update!

coatless · 2022-01-15T21:36:40Z

tests/testthat/test-fatal-errors.R

@@ -1,12 +1,10 @@
 test_that("Unbalanced braces", {


coatless · 2022-01-15T21:37:54Z

DESCRIPTION

@@ -1,5 +1,5 @@
 Package: bibtex
-Version: 0.4.3
+Version: 0.4.3.9000


Let's go 0.5.0 for the next CRAN release and, then, bump to 1.0.0.

coatless · 2022-01-15T21:39:38Z

revdep/README.md

+|magrittr |2.0.1   |NA         |*  |
+|stringi  |1.7.6   |NA         |*  |
+|stringr  |1.4.0   |NA         |*  |
+


I'm not sure there are any dependencies w.r.t. stringr / stringi / glue / magrittr?

This one is funny: On CRAN version, bibtex imports stringr (¿?), that imports glue, magritt and stringi, so I guess thats why they appear on the summary

https://cran.r-project.org/web/packages/bibtex/index.html
https://cran.r-project.org/web/packages/stringr/index.html

dieghernan · 2022-01-16T08:32:09Z

Thanks for this useful review. I would set this as a Draft until I tackle all your comments

Co-authored-by: James J Balamuta <coatless@users.noreply.github.com>

dieghernan · 2022-01-17T09:30:47Z

Hi @coatless , I think I tackled all your comments, ready for review

dieghernan · 2022-02-23T21:49:20Z

Hi @coatless , just a quick reminder on this

maelle · 2022-09-16T07:36:05Z

👋 @coatless @dieghernan, what's the status on this? Thanks both for your work on {bibtex} 🙏

Some snapshots changes due to changes on toBibTex() on later versions of R, not related with the package

dieghernan · 2022-09-16T09:11:05Z

Hi @maelle

I re-runned checks, examples and snapshots after 8 months and I see no problems, so this is ready on my side. This PR would close #16 and close #42

maelle · 2022-09-16T10:10:36Z

Thanks @dieghernan!
@coatless would you be able to review this PR?

coatless · 2022-09-23T02:08:03Z

@dieghernan thanks for addressing the remaining issues. Sorry this sat for so long!

dieghernan added 25 commits December 4, 2021 10:53

Upgrade testing suite to testthat 3

9a3dccd

Add testing for previous issues

0918cfd

#45

Add tests for standard bibtex entries

4053f15

As defined in BibTEX version 0.99b https://ctan.javinator9889.com/biblio/bibtex/base/btxdoc.pdf

Update actions

db4e1a9

Add testing for examples

b99129b

Add snapshots for examples read.bib This may fail on some platforms

22459a1

Skip on non windows

9aebbc9

Possibly a character problem related with #20 and #43

Not test on R 3.4

f2772bb

Some changes in default parsing (snapshot), but results are still ok

Add more tests

ef9c6b6

Try to increase coverage

2325dfd

One more tests for do_read_bib

f56c65e

Fix test for do_read_bib

408cd89

Revert actions

73d0303

Add devtools for testing

c46a194

Add more tests

143fdaf

Add test for multiline string

b2b1417

Add non standard field names

9dc73c3

Add myself as author

83cfe61

Move issues to inst/bib files

24ed42c

Refactor tests for avoiding cluttering

a98400a

Modify do_read_bib()

698c647

Also bump version

Remove C code :)

8f0e88d

Recreate snapshots on Linux

e3e2ebe

Check reverse dependencies

b37df5f

Merge main branch

4bc3b23

Merge branch 'master' into phase2 # Conflicts: # tests/testthat/_snaps/examples.md # tests/testthat/test-fatal-errors.R

dieghernan mentioned this pull request Jan 14, 2022

Improve test suite for bibtex #46

Merged

dieghernan commented Jan 14, 2022

View reviewed changes

coatless reviewed Jan 15, 2022

View reviewed changes

Add backports

d3dcc96

dieghernan marked this pull request as draft January 16, 2022 08:32

dieghernan and others added 8 commits January 16, 2022 10:01

Deprecate arguments

3b2ad27

Use seq_along

674527d

Co-authored-by: James J Balamuta <coatless@users.noreply.github.com>

Use seq_along again

648f0ca

Co-authored-by: James J Balamuta <coatless@users.noreply.github.com>

Remove completely header and footer

b88d583

Document internal functions

769aac3

Rename internal params

f822c4b

Use vapply and new tests

876f3da

Rerun revdeps

522042d

dieghernan marked this pull request as ready for review January 17, 2022 09:30

dieghernan mentioned this pull request Jan 17, 2022

fatal flex scanner internal error--end of buffer missed #16

Closed

dieghernan added 3 commits September 16, 2022 10:02

Update docs and snapshots

21c60ec

Update revdep and check action

b752157

Some snapshots changes due to changes on toBibTex() on later versions of R, not related with the package

Fix action and snapshots

052ff08

coatless approved these changes Sep 23, 2022

View reviewed changes

coatless merged commit e6f9720 into ropensci:master Sep 23, 2022

coatless mentioned this pull request Sep 26, 2022

Proposal: Improving the package #45

Closed

14 tasks

	make.citation.list <- function( x, header, footer){
	rval <- list()
	for( i in seq_along(x) ){
	if( !is.null(x[[i]] ) )
	rval <- c( rval, x[[i]] )
	}
	class(rval) <- c( "bibentry" )
	rval
	}

	@book{brown:1963,
	author = "Brown, R. G.",
	year = 1963,
	publisher = "Prentice-Hall"
	}

	@misc{murdoch:2010,
	author = "Duncan Murdoch",
	title = {"Parsing {Rd} files unbalanced",
	year = 2010,
	url = "http://developer.r-project.org/parseRd.pdf"
	}

Remove C code #47

Remove C code #47

Conversation

dieghernan commented Jan 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dieghernan commented Jan 14, 2022

coatless left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dieghernan Jan 16, 2022 • edited Loading

Choose a reason for hiding this comment

dieghernan commented Jan 16, 2022

dieghernan commented Jan 17, 2022

dieghernan commented Feb 23, 2022

maelle commented Sep 16, 2022

dieghernan commented Sep 16, 2022

maelle commented Sep 16, 2022

coatless commented Sep 23, 2022

dieghernan commented Jan 14, 2022 •

edited

Loading

dieghernan Jan 16, 2022 •

edited

Loading