Full package review #10
Conversation
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [peter-evans/create-pull-request](https://github.com/peter-evans/create-pull-request) from 6 to 7. - [Release notes](https://github.com/peter-evans/create-pull-request/releases) - [Commits](peter-evans/create-pull-request@v6...v7) --- updated-dependencies: - dependency-name: peter-evans/create-pull-request dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]>
Bumps [JamesIves/github-pages-deploy-action](https://github.com/jamesives/github-pages-deploy-action) from 4.6.3 to 4.6.4. - [Release notes](https://github.com/jamesives/github-pages-deploy-action/releases) - [Commits](JamesIves/github-pages-deploy-action@v4.6.3...v4.6.4) --- updated-dependencies: - dependency-name: JamesIves/github-pages-deploy-action dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]>
Bumps [JamesIves/github-pages-deploy-action](https://github.com/jamesives/github-pages-deploy-action) from 4.6.4 to 4.6.8. - [Release notes](https://github.com/jamesives/github-pages-deploy-action/releases) - [Commits](JamesIves/github-pages-deploy-action@v4.6.4...v4.6.8) --- updated-dependencies: - dependency-name: JamesIves/github-pages-deploy-action dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]>
|
This pull request:
Reach out on slack ( (Note that results may be inaccurate if you branched from an outdated version of the target branch.) |
There was a problem hiding this comment.
Thanks for starting this package @Bisaloo 🙌 I can see many use cases where people want to parse these fields where this will prove helpful 😊
Without having any preexisting knowledge about the functionality in this package, I was slightly confused about the double functions: One for Authors@R and one for Authors. I would not be able to reliably say a priori which one to use for any given package, or whether it matters which one I use. This makes me wonder whether we want to provide a wrapper to check this for the user instead? (in the future) This way the user would only need to run one function to get the information, if they want.
I also wonder about contributor roles, ORCIDs, emails and any other additional information that may be in the author field. Right now, as far as I can see, these are not being parsed and separated. Does this not obstruct the idea of expanding names, and creating the authorative author information across various packages?
I recognize this is a work in progress, so I recognize some of these things may already be on your mind. :-) I like the package and I can see it being used in quite some GitHub actions or workflows throughout the R ecosystem. If any of my comments are unclear, please let me know – I tried to be concise but I notice that it's the first time I am looking at this package.
| pkg_description <- system.file("DESCRIPTION", package = "authoritative") | ||
| authors_r_pkg <- read.dcf(pkg_description, "Authors@R") |
There was a problem hiding this comment.
Might we want to provide a wrapper for these steps, to make it easier to use? Seems like a common action and we can scaffold the usage for success with it, maybe.
| #' @examples | ||
| #' # Read from a DESCRIPTION file directly | ||
| #' utils_description <- system.file("DESCRIPTION", package = "utils") | ||
| #' utils_authors <- read.dcf(utils_description, "Author") |
There was a problem hiding this comment.
I just spent five minutes figuring out why I couldn't get this for linelist, and now I realized that it is single here and plural for @R 😅
| #' sort() | ||
| #' | ||
| #' @export | ||
| parse_authors <- function(author_string) { |
There was a problem hiding this comment.
| parse_authors <- function(author_string) { | |
| parse_author <- function(author_string) { |
Maybe to be in line with the description field?
| ) | ||
|
|
||
| string |> | ||
| stringi::stri_replace_all_regex( |
There was a problem hiding this comment.
Trying to use this creates rather opaque warnings that may seem ominous to new users.
Warning message:
In stringi::stri_replace_all_regex(string, paste0("\\", bracket, :
argument is not an atomic vector; coercing
Can we clarify these?
There was a problem hiding this comment.
I understand the functionality itself, but I don't yet see how this would be used in relation to the author information. Might be good to present this in a vignette.
| @@ -0,0 +1,32 @@ | |||
| test_that("remove_brackets() works in simple cases", { | |||
|
|
|||
| expect_identical( | |||
There was a problem hiding this comment.
I was hoping to see these tests also relate to the integration of remove_brackets() with the other functions. Maybe we can add this at a later stage.
There was a problem hiding this comment.
Here I also get the functionality, which is great, and I do not yet see how I would use this in relation to the other functions. Right now, if I parse names, I end up with contributor roles and ORCIDs included as well. I expect that would interfere with this operation, if we do not do further parsing of the author fields to handle those additional fields. Maybe this is what the remove_brackets() is supposed to do though 🤔
What do you think?
This full package review was requested by @Bisaloo directly, so I am creating a PR so that I can leave precise comments for his convenience 😊