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

bib2df #124

Closed
11 of 14 tasks
ottlngr opened this issue Jun 6, 2017 · 37 comments
Closed
11 of 14 tasks

bib2df #124

ottlngr opened this issue Jun 6, 2017 · 37 comments

Comments

@ottlngr
Copy link

ottlngr commented Jun 6, 2017

Summary

  • What does this package do? (explain in 50 words or less):

Provides functionality to parse BibTeX files to data.frame which can be used for analyses and visualizations using common tools like dplyr, tidyr and ggplot2. Changes in the data.frame can be written back to a valid BibTeX file.

  • Paste the full DESCRIPTION file inside a code block below:
Package: bib2df
Type: Package
Title: Parse a BibTeX File to a data.frame
Version: 0.2
Date: 2017-05-21
Authors@R: c(person("Philipp", "Ottolinger", email = "philipp@ottolinger.de", role = c("aut", "cre")),
             person("Thomas", "Leeper", email = "thosjleeper@gmail.com", role = "ctb"),
             person("Maëlle", "Salmon", email = "maelle.salmon@yahoo.se", role = "ctb"))
Description: Parse a BibTeX file to a data.frame to make it accessible for further analysis and visualization.
URL: https://github.com/ottlngr/bib2df
BugReports: http://github.com/ottlngr/bib2df/issues
License: GPL-3
LazyData: TRUE
Imports:
    dplyr,
    plyr,
    stringr,
    humaniformat
Suggests:
    testthat,
    knitr,
    rmarkdown,
    ggplot2,
    tidyr
RoxygenNote: 6.0.1
VignetteBuilder: knitr
  • URL for the package (the development repository, not a stylized html page):

https://github.com/ottlngr/bib2df

  • Please indicate which category or categories from our package fit policies this package falls under and why? (e.g., data retrieval, reproducibility. If you are unsure, we suggest you make a pre-submission inquiry.):

Data extraction, because bib2df makes entries of a BibTeX file accessible in the most common data structure, the data.frame.

  • Who is the target audience?

Anyone who wants to gain insights in the characteristics and coherences of bibliographic references.

  • Are there other R packages that accomplish the same thing? If so, what is different about yours?

There are a few packages that can deal with BibTeX in general, like RefManageR#119 and others mentioned there. These packages are very close to the actual purpose of BibTeX, providing special classes and can be used as reference managing software. bib2df is not for managing references, but for extracting the data to a common data structure. To my knowledge, only bib2df offers parsing to data.frame.

Requirements

Confirm each of the following by checking the box. This package:

  • does not violate the Terms of Service of any service it interacts with.
  • has a CRAN and OSI accepted license.
  • contains a README with instructions for installing the development version.
  • includes documentation with examples for all functions.
  • contains a vignette with examples of its essential functions and uses.
  • has a test suite.
  • has continuous integration, including reporting of test coverage, using services such as Travis CI, Coeveralls and/or CodeCov.
  • I agree to abide by ROpenSci's Code of Conduct during the review process and in maintaining my package should it be accepted.

Publication options

  • Do you intend for this package to go on CRAN? It already is on CRAN.
  • Do you wish to automatically submit to the Journal of Open Source Software? If so:
    • The package contains a paper.md with a high-level description in the package root or in inst/.
    • The package is deposited in a long-term repository with the DOI:
    • (Do not submit your package separately to JOSS)

Detail

  • Does R CMD check (or devtools::check()) succeed? Paste and describe any errors or warnings:
R CMD check results
0 errors | 0 warnings | 0 notes
  • Does the package conform to rOpenSci packaging guidelines? Please describe any exceptions:

  • If this is a resubmission following rejection, please explain the change in circumstances:

  • If possible, please provide recommendations of reviewers - those with experience with similar packages and/or likely users of your package - and their GitHub user names:

@sckott
Copy link
Contributor

sckott commented Jun 10, 2017

Editor checks:

  • Fit: The package meets criteria for fit and overlap
  • Automated tests: Package has a testing suite and is tested via Travis-CI or another CI service.
  • License: The package has a CRAN or OSI accepted license
  • Repository: The repository link resolves correctly
  • Archive (JOSS only, may be post-review): The repository DOI resolves correctly
  • Version (JOSS only, may be post-review): Does the release version given match the GitHub release (v1.0.0)?

Editor comments

Thanks for your submission @ottlngr !

Currently seeking reviewers. It's a good fit and not overlapping.

  • goodpractice::gp() output is below. Some things for submitter and reviewers to consider.
It is good practice towrite unit tests for all functions, and all package code in general. 91% of
    code lines are covered by test cases.

    R/bib2df_gather.R:20:NA
    R/bib2df_tidy.R:23:NA
    R/bib2df_tidy.R:30:NA
    R/df2bib.R:20:NA
    R/df2bib.R:21:NA
    ... and 3 more linesomit "Date" in DESCRIPTION. It is not required and it gets invalid quite
    often. A build date will be added to the package when you perform `R CMD build` on it.avoid long code lines, it is bad for readability. Also, many people prefer
    editor windows that are about 80 characters wide. Try make your lines shorter than 80
    characters

    inst/legacy/bib2df_explore.R:2:1
    inst/legacy/bib2df_explore.R:3:1
    inst/legacy/bib2df_scrape_l.R:34:1
    inst/legacy/bib2df_scrape_l.R:39:1
    R/bib2df_gather.R:23:1
    ... and 8 more linesavoid sapply(), it is not type safe. It might return a vector, or a list,
    depending on the input data. Consider using vapply() instead.

    inst/legacy/bib2df_gather2_l.R:2:12
    inst/legacy/bib2df_scrape_l.R:6:9
    inst/legacy/bib2df_scrape_l.R:37:40not import packages as a whole, as this can cause name clashes between the
    imported packages. Instead, import only the specific functions you need.avoid 'T' and 'F', as they are just variables which are set to the logicals
    'TRUE' and 'FALSE' by default, but are not reserved words and hence can be overwritten
    by the user.  Hence, one should always use 'TRUE' and 'FALSE' for the logicals.

    R/bib2df_gather.R:NA:NA
    R/bib2df_tidy.R:NA:NA
    R/bib2df_tidy.R:NA:NA

Reviewers: @vasantm @adamhsparks
Due date: 2017-07-03

@ottlngr
Copy link
Author

ottlngr commented Jun 11, 2017

Updated the package regarding to the gp() output. This is what remains:

It is good practice to

  ✖ write unit tests for all functions, and all package code in general. 87% of code
    lines are covered by test cases.

    R/bib2df_gather.R:20:NA
    R/bib2df_tidy.R:25:NA
    R/bib2df_tidy.R:26:NA
    R/bib2df_tidy.R:27:NA
    R/bib2df_tidy.R:28:NA
    ... and 18 more lines

  ✖ not import packages as a whole, as this can cause name clashes between the imported
    packages. Instead, import only the specific functions you need.

@sckott
Copy link
Contributor

sckott commented Jun 12, 2017

thanks @ottlngr , one reviewer found, one more to go

@adamhsparks
Copy link
Member

adamhsparks commented Jun 15, 2017

I've completed my review of bib2df, here are my comments and suggestions.

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

  • As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

  • A statement of need clearly stating problems the software is designed to solve and its target audience in README
  • Installation instructions: for the development version of package and any non-standard dependencies in README
  • Vignette(s) demonstrating major functionality that runs successfully locally
  • Function Documentation: for all exported functions in R help
  • Examples for all exported functions in R Help that run successfully locally
  • Community guidelines including contribution guidelines in the README or CONTRIBUTING, and URL, Maintainer and BugReports fields in DESCRIPTION

Functionality

  • Installation: Installation succeeds as documented.
  • Functionality: Any functional claims of the software been confirmed.
  • Performance: Any performance claims of the software been confirmed.
  • Automated tests: Unit tests cover essential functions of the package
    and a reasonable range of inputs and conditions. All tests pass on the local machine.
  • Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Final approval (post-review)

  • The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing:

Total time to review was about 3.5 hours.


Review Comments

bib2df offers functionality for working with LaTeX .bib files in R as a data.frame object. This functionality is well appreciated and will serve many people well. I'm actually using it at the moment in a paper that I'm writing on reproducibility in plant pathology, https://github.com/phytopathology/Reproducible.Plant.Pathology. The package installs quickly with no issues from CRAN or from source building the package in RStudio. The vignette and README offer good examples of how it may be used, but the package itself doesn't attempt to do more than convert a .bib file to a data.frame() object. I appreciate that @ottlngr has created something that is simple and does one job and does it well.

General Comments

  • Passes checks with no NOTES or WARNINGS on macOS Sierra, R 3.4.0

  • Line widths are more than 80 chars in several of the .R files, using "CMD+^+A"
    will reformat the code in RStudio and wrap long lines. However, some may need to be manually shortened.

  • Please check spelling using devtools::spell_check

  • Please check spelling in vignette as well. RStudio offers built-in spell-check for Rmd files, .

  • Commas should always have a space after, check using lintr::lint_package()

  • Variables and function names should all be lowercase, e.g., df2bib.R line 19

  • The details portion of bib2df() documentation includes,

Uses the internal bib2df_read(), bib2df_gather() and bib2df_tidy(). No magic, just a wrapper.

I'm not sure that this is necessary to include for the end users, but since you have documented these internal functions you can link to the help file for them. I would suggest not including this information here.

  • Following on to the previous point, I'd suggest not exposing these functions outside the package namespace and not documenting them. If I understand correctly, bib2df_read(), bib2df_gather() and bib2df_tidy() are all internal functions that the end-user is unlikely to interact directly with. As the package is written now, I don't see how or why a user would, so I suggest not documenting in the help files and not exporting them.

Functions

  • Using bib2df without specifying separate_names = results in a warning message. This should not happen, especially as bib2df(path) is the example and triggers this behaviour. I'd default to FALSE with the option to set to TRUE, but this really is your choice how you think it should be handled by default.

  • Likewise, bib2df_tidy() exhibits the same behaviour and should not return values with a WARNING message.

  • Using bib2df_tidy() with separate_names set to TRUE or FALSE results in an error message, e.g.,

    bib <- bib2df_tidy(bib, separate_names = FALSE)
    Show Traceback
    
    Rerun with Debug
    Error in mutate_impl(.data, dots) :
    Evaluation error: non-character argument.

    However, as noted, I'm not sure that this function should be exported and exposed to the user?

Documentation

  • The bib2df-package help file could use some more formatting. You could use \code{\link{dplyr}} and the like for the other packages used. data.frame is not wrapped in a \code{} tag here either.

  • The title field in .Rd files should not end in a period, the following files do and should be corrected,

    • bib2df.R

    • bib2df_gather.R

    • bib2df_tidy.R

    • bib2df_read.R

    • df2bib.R

  • I'd change the documentation to say that this package returns a tibble, not a data.frame.

Vignette

  • The vignette is not indexed so when searching for it, vignette(bib2df), the result is,

    Warning message:
    vignettebib2dfnot found
  • Please make sure that the %\VignetteIndexEntry{} is updated, e.g., %\VignetteIndexEntry{bib2df}

  • Why is it advised to download a bib file? Why not read from a URL? Some more detail in the vignette about this would be welcome. Minor point, but is a shorter URL for the example bib file possible?

  • A link to the humaniformat package would be nice.

NEWS.md

  • Formatting of functions and R objects is inconsistent. Some use the code tags,
    others don't.

Packages Used

  • plyr is no longer actively developed, and since dplyr is already imported and has bind_rows I'd suggest using this function. This also reduces dependencies by one package.

Tests

  • Tests run successfully on macOS Sierra, R 3.4.0

  • Tests cover 87% bib2df_tidy.R and df2bib.R have the least coverage according to covr.io, if tests could be written to cover these more completely it would be nice.

@sckott
Copy link
Contributor

sckott commented Jun 15, 2017

thanks for your review @adamhsparks !

@ottlngr
Copy link
Author

ottlngr commented Jun 16, 2017

Thanks for your extensive and detailed feedback, @adamhsparks. I will bundle your remarks in issues soon and process them as soon as possible.

@ottlngr
Copy link
Author

ottlngr commented Jul 10, 2017

Don't want to be meticulous, but @vasantm's review is one week overdue now.

@sckott
Copy link
Contributor

sckott commented Jul 10, 2017

@vasantm please get your review in asap - if you can't do it let me know asap

@vasantm
Copy link

vasantm commented Jul 11, 2017

@sckott
Copy link
Contributor

sckott commented Jul 11, 2017

thanks @vasantm

@sckott
Copy link
Contributor

sckott commented Jul 14, 2017

@vasantm Do you think you can get your review in soon?

@sckott
Copy link
Contributor

sckott commented Jul 17, 2017

@vasantm Please get your review in soon. If you cannot, please let me know and I will find someone else

@sckott
Copy link
Contributor

sckott commented Jul 25, 2017

@ottlngr Sorry about the wait - I'll go ahead and do the 2nd review - hopefully I can get it done tomorrow.

@ottlngr
Copy link
Author

ottlngr commented Jul 26, 2017

Thanks, @sckott.

@sckott
Copy link
Contributor

sckott commented Aug 2, 2017

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

  • As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

  • A statement of need clearly stating problems the software is designed to solve and its target audience in README
  • Installation instructions: for the development version of package and any non-standard dependencies in README
  • Vignette(s) demonstrating major functionality that runs successfully locally
  • Function Documentation: for all exported functions in R help
  • Examples for all exported functions in R Help that run successfully locally
  • Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

Functionality

  • Installation: Installation succeeds as documented.
  • Functionality: Any functional claims of the software been confirmed.
  • Performance: Any performance claims of the software been confirmed.
  • Automated tests: Unit tests cover essential functions of the package
    and a reasonable range of inputs and conditions. All tests pass on the local machine. Coverage is at 87% - quite good, eventually work on getting full coverage
  • Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

some bits from guidelines:
- In releases https://github.com/ottlngr/bib2df/releases - we do want maintainers to put the relevant news items for each cran release in a git tagged release (you can link directly to issues as well, which is really helpful to give users context). Note that you can retroactively git tag commits, and add news items to releases.
- Example are sparse. Granted, the functions I suppose are relatively simple but should have many more covering various use cases.

Final approval (post-review)

  • The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing:

2


Review Comments

Nice package!

  • i wouldn't run installation of pkgs in your vignettes, both for install.packages and devtools::install_github
  • for vignette: i'd suggest warnings=FALSE and messages=FALSE
  • for both functions have you considered vectorizing? allowing >1 things input (either file paths or data.frames? not sure if you have already considered and didn't make sense. Maybe could be different functions if you don't want to break functionality of current fxns.

df2bib

  • should use @references to link to URLs describing .bib files
  • i'd think that this function would allow the user to append or not to a file, toggling with a parameter. e.g. cat(..., append = TRUE)
  • you have two helper fxns inside this fxn: capitalize and na_replace - i'd put those outside of the function itself to simplify
  • seems like it should fail better when passing in things that are not file paths, e.g.,
df2bib(bib, 5)
#> Error in cat(paste0("@", capitalize(x$Category), "{", x$Bibtexkey, ",\n",  : 
#>   invalid connection

bib2df

  • @return should say that it returns a tibble, not a data.frame
  • should note that when separate_names = TRUE that it gives back a list of data.frame's and when FALSE a list of character strings
  • As other review raised above, It's good idea to support reading from a URL. Does seem to work, e.g, bib2df("https://raw.githubusercontent.com/ottlngr/bib2df/master/inst/extdata/biblio.bib"),
  • how are malformed bib files dealt with now? e.g., with the following malformed bib
@Article{Binmore2008,
  Title = {Do Conventions Need to Be Common Knowledge?},
  Author = {Binmore, Ken},
  Journal = {Topoi},
  Year = {2008},
  Number = {1},
  Pages = {17--27},
  Volume = {27}
}


@Book{Osborne1994,
  Title = {A Course in Game Theory},
  Author = {Osborne, Martin J. and Rubinstein, Ariel},
  Publisher = {The MIT Press, Cambridge, Massachusetts},
  Year = {1994}
}


InCollection{BrandenburgerDekel1989,
  Title = {The Role of Common Knowledge Assumptions in Game Theory},
  Author = {Brandenburger, Adam and Dekel, Eddie},
  Booktitle = {The Economics of Missing Markets, Information and Games},
  Publisher = {Oxford University Press},
  Year = {1989},
  Address = {New York},
  Chapter = {3},
  Editor = {Hahn, Frank},
  Pages = {46--61}

your function drops the invalid entry (last one) silently

> bib2df("stuff2.bib")
# A tibble: 2 x 30
  CATEGORY   BIBTEXKEY  ADDRESS ANNOTE    AUTHOR                                               BOOKTITLE CHAPTER
     <chr>       <chr>    <chr>  <chr>    <list>                                                   <chr>   <chr>
1  ARTICLE Binmore2008     <NA>   <NA> <chr [1]>                                                    <NA>    <NA>
2     BOOK Osborne1994 New York   <NA> <chr [2]> The Economics of Missing Markets, Information and Games       3
# ... with 23 more variables: CROSSREF <chr>, EDITION <chr>, EDITOR <list>, HOWPUBLISHED <chr>, INSTITUTION <chr>,
#   JOURNAL <chr>, KEY <chr>, MONTH <chr>, NOTE <chr>, NUMBER <chr>, ORGANIZATION <chr>, PAGES <chr>, PUBLISHER <chr>,
#   SCHOOL <chr>, SERIES <chr>, TITLE <chr>, TYPE <chr>, VOLUME <chr>, YEAR <dbl>, TITLE.1 <chr>, AUTHOR.1 <chr>,
#   PUBLISHER.1 <chr>, YEAR.1 <chr>

while bibtex package errors on invalid entry

bibtex::read.bib("stuff2.bib")
#> Error: Invalid bib file
  • Seems like handling malformed values for certain things could be better. Although maybe not common, users could have e.g., a character string like foobar instead of 1989 for year - leading to NA's - some type checking could be done on certain inputs, or is that considered beyond scope here?

@sckott
Copy link
Contributor

sckott commented Aug 2, 2017

@ottlngr both reviews in now

let me know if you have any questions about my review or the process in general

@ottlngr
Copy link
Author

ottlngr commented Aug 3, 2017

Thanks for your review and useful comments, @sckott.

Seems like handling malformed values for certain things could be better. Although maybe not common, users could have e.g., a character string like foobar instead of 1989 for year - leading to NA's - some type checking could be done on certain inputs, or is that considered beyond scope here?

I would not say it is beyond scope. But when creating bib2df my intention was not to build a tool to manage .bib files and especially not to build a tool that manages and "repairs" other people's .bib files. So I think it would be useful to have some type checking and to throw corresponding warnings. Rejecting a file or an entry because of malformed values would undermine bib2df's aspiration to parse any .bib file. Another option would be to use character columns when non-numeric values are detected in actual numeric fields.

Will have to think about that a bit more, but there will be some kind of solutions for this as well as for the rejecting of whole entries.

@sckott
Copy link
Contributor

sckott commented Aug 4, 2017

Sounds good @ottlngr - i can see the argument against verifying bib files - just nudging a bit in direction of helping users a bit more

@ottlngr
Copy link
Author

ottlngr commented Aug 7, 2017

@sckott

for both functions have you considered vectorizing? allowing >1 things input (either file paths or data.frames? not sure if you have already considered and didn't make sense. Maybe could be different functions if you don't want to break functionality of current fxns.

I think this makes sense for the .bib -> data.frame task, e.g. when read many .bibfiles from a directory. For the other way, data.frame -> .bib I do not see a use case. Even if you have read several .bib files with bib2df you can still bind these data.frames together and export them back into a single .bib file. And the use case with exporting several data.frames to several .bib files is some kind of "BibTeX Managing"-Task and I don't see bib2df pushing forward in that direction to much.

And the first case I described, reading multiple .bibfiles with bib2df(), is so simple using lapply() (could be demonstrated in the vignette) so I do not see the necessity to write a wrapper for this task. Do you think different?

Example are sparse. Granted, the functions I suppose are relatively simple but should have many more covering various use cases.

Do you think of more examples in the documentation or in the vignette? Or both?

How does the final review process work? Do I have to provide some code that demonstrates my changes?

@sckott
Copy link
Contributor

sckott commented Aug 7, 2017

And the first case I described, reading multiple .bibfiles with bib2df(), is so simple using lapply() (could be demonstrated in the vignette) so I do not see the necessity to write a wrapper for this task. Do you think different?

true, it is easy to do. do show how to do it

Do you think of more examples in the documentation or in the vignette? Or both?

i think just in docs

How does the final review process work? Do I have to provide some code that demonstrates my changes?

There isn't one way to do this. When i submit my own packages for review, I find it easiest to open an issue in the repo for each issue raised by a reviewer - and I label with review - then link to the issue in the review thread - You don't have to go that far: at least give a summary of what you changed/didn't change/etc. just as you would for manuscript reviews.

@ottlngr
Copy link
Author

ottlngr commented Aug 30, 2017

Sorry for the delay! Here is what I achieved based on your reviews:

  • Improved formatting, respected code conventions and spell checked the package
  • Avoided unnecessary dependencies (plyr)
  • Hid internal functions from the user by not exporting them
  • Moved helper functions to helpers.R
  • Avoided warning messages and improved error handling
  • Improved the vignette
  • Linked to the humaniformat package and to a BibTex reference
  • df2bib2() now supports appending to existent files
  • Improved error messages when reading from or writing to invalid file paths or URLs
  • Added a message when a malformed .bib file is recognized as such while reading

I tried to document each step in a single commit with a somewhat meaningful commit message: https://github.com/ottlngr/bib2df/commits/master

If you have any questions or want some kind of demonstration of specific (new) features just let me know.

Thanks to @adamhsparks and @sckott again your helpful and detailed reviews!

@adamhsparks
Copy link
Member

adamhsparks commented Aug 30, 2017

Thanks @ottlngr, this all sounds great. I'll have a look at what you've done this weekend.

@sckott
Copy link
Contributor

sckott commented Aug 31, 2017

Thanks very much @ottlngr - will have a look soon and get back to you

@adamhsparks
Copy link
Member

adamhsparks commented Sep 2, 2017

Thanks for your efforts to address my comments, @ottlngr. There are some issues to be addressed first before I can check off on accepting this package.

Comments in code

  1. Due to issues with covr on my machine, I can't use the goodpractice package, but I can still run lintr::lint_package(), which raises several easily correctable flags. Correct any flags that this raises. Nothing major, but it will help clean up the package code.

  2. I'm not clear from the documentation about whether this package is centred around tibble() or data.frame()? The vignette suggests that it's a normal data.frame() but the function, bib2df() returns tibble()?

  3. I see RCurl is used in the package now. I'd suggest not using that package per the rOpenSci guidelines for preferred packages. This would be a good place to use httr::http_error().

The vignette

  1. Check spelling in the vignette using the built-in spellcheck in RStudio for Rmd files.

  2. You switch between "BibTex" and "BibTeX" in the vignette. You should use "BibTeX".

  3. It's a nitpick, but in the vignette, you say that the packages in the tidyverse use the data.frame as a data structure. Maybe I'm nitpicking because a tibble is a form of a data.frame, but I tend to think that the tidyverse uses tibbles. I don't see how the tidyverse is necessary here. You can simply state that R uses data.frame objects as one of it's basic data structures unless there is a compelling reason to mention the tidyverse. I'm not against the tidyverse per se, but I don't see what it adds to the documentation here to call it out specifically. Since you are returning a tibble, you could highlight that you're using the latest and greatest version of a data.frame in R that is also used in the tidyverse.

  4. In the vignette you state:
    "Since all the BibTeX entries are represented by rows in a data.frame, all entries can be altered just like one would alter the value of a certain cell in a data.frame".
    Well, yes. But you seem to suggest that a data.frame from bib2df is not a normal data.frame object, I guess it is a tibble, but this type of language is unclear to me. I'd suggest rephrasing this to avoid confusion.

@ottlngr
Copy link
Author

ottlngr commented Sep 3, 2017

Thanks, @adamhsparks for the feedback, especially for the hint to use httr::http_error()!

goodpractice::gp() still returns missing unit tests, but I'm working on that.

I agreed to all your comments, see the commits below for details:

ropensci/bib2df@d21428e
ropensci/bib2df@eb02636
ropensci/bib2df@acfcd82
ropensci/bib2df@f426d5d
ropensci/bib2df@b4fbe67
ropensci/bib2df@312a6b3
ropensci/bib2df@befd7dc
ropensci/bib2df@6f65e34
ropensci/bib2df@afd0aff
ropensci/bib2df@cfc8426

@adamhsparks
Copy link
Member

adamhsparks commented Sep 3, 2017

Thanks @ottlngr.

Line 65 of README.Rmd, please correct the spelling of "Community"

## Comunity Guidelines

Aside from that, looks good to me. I've checked the approval box in my original review.

@ottlngr
Copy link
Author

ottlngr commented Sep 4, 2017

Sorry for the typo - just noticed I had not knitted the .Rmd but now all changes are in place: ropensci/bib2df@4ca592b

Thanks for the approval, all in all a very helpful and educational process!

@sckott
Copy link
Contributor

sckott commented Sep 5, 2017

@ottlngr

  • instead of commenting out install.packages in your vignette, i meant to say use {r eval=FALSE} in the knit chunk - so for the reader they can run it easily, but it's not actually run during the build process

Looks good otherwise.

@adamhsparks
Copy link
Member

adamhsparks commented Sep 5, 2017

I agree with @sckott on this point. You can silently load bib2df elsewhere without echoing the code so that the library loads to execute the examples that follow.

@ottlngr
Copy link
Author

ottlngr commented Sep 6, 2017

Thanks for clarification. I had a look at other packages' vignettes - I removed the install.packages() and load the package with echo=FALSE,message=FALSE. I guess you're ok with that?

@sckott
Copy link
Contributor

sckott commented Sep 6, 2017

@ottlngr sorry, i meant eval = FALSE - still want to echo it so the user sees that they probably need to isntall the pkg brefore using it 😄

@ottlngr
Copy link
Author

ottlngr commented Sep 6, 2017

I'm totally confused now about that simple task 😂

I had a look at other vignettes (like https://github.com/ropensci/tabulizer/blob/master/vignettes/tabulizer.Rmd), many do not put install.packages() inside the vignette. But if one does, eval = FALSE is the chunk option of choice, @sckott? This makes sense by all means.

And the library() command without any chunk option then?

@sckott
Copy link
Contributor

sckott commented Sep 6, 2017

Yes, that's right, install.packages in a chunk with eval = FALSE, and library in a chunk that does run (eval = TRUE the default)

@ottlngr
Copy link
Author

ottlngr commented Sep 6, 2017

😅

ropensci/bib2df@d401345

@sckott
Copy link
Contributor

sckott commented Sep 14, 2017

Approved! thank for your submission @ottlngr !

  • Please transfer the package to ropensci- I've invited you to a team within our ropensci organization. After you accept you should be able to move it.
  • Make sure to
    • add rOpenSci footer to README
      [![ropensci_footer](https://ropensci.org/public_images/ropensci_footer.png)](https://ropensci.org)
    • Change any needed links, such those for CI badges
    • Travis CI should update to the new location automatically - you may have to update other CI systems manually
  • Are you interested in doing a blog post for our blog https://ropensci.org/blog/ ? either a short-form intro to it (https://ropensci.org/tech-notes/) or long-form post with more narrative about its development (https://ropensci.org/blog/). If so, i'll get you the details on that

@ottlngr
Copy link
Author

ottlngr commented Sep 15, 2017

Thanks for guiding me through the whole process, @adamhsparks and @sckott !

I will transfer the repo to rOpenSci this evening (CEST). As soon as the transfer is completed, I will make the needed changes.

I'm definitely interested in writing a post for https://ropensci.org/tech-notes/, and maybe even a long-form post for https://ropensci.org/blog - depends on the existence of a suitable narrative and enough spare time to not delay the publication.

@sckott
Copy link
Contributor

sckott commented Sep 15, 2017

Thanks, sounds good. Okay, just let us know on the post either way.

@sckott sckott closed this as completed Sep 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants