Reply detox #111
Reply detox #111
Conversation
…e mime type multiple times, it is just counted once. Also, the percentage is calculated relative to the overall number of Crossref records #66
Merge branch 'reply_detox' of https://github.com/subugoe/hybridmdpackage into reply_detox # Conflicts: # R/metrics_tdm.R
…astula email is rendered
|
Is somehow ready |
| @@ -10,7 +10,7 @@ | |||
| get_cr_md <- function(dois, .progress = "text") { | |||
| checkmate::assert_character(dois, any.missing = FALSE, unique = TRUE) | |||
| tt <- rcrossref::cr_works(dois = dois, .progress = .progress)[["data"]] | |||
| if (!is.null(tt)) { | |||
| if (nrow(tt) != 0) { | |||
There was a problem hiding this comment.
this makes the function not length-stable, but we'll fix these APIs later.
Length-stably, get_cr_md() should either return a df with 0 rows, or it should throw an error if we think that it makes more sense (I'd lean towards error).
In that case, the responsibility to pass on dois which actually have metadata on cr falls to upstream functions (as in biblids).
Anyway, this hotfixes #113 for now.
R/email.R
Outdated
| md_data_attachment <- function(.md = NULL, session_id = NULL, dois = NULL) { | ||
| if(is.null(.md)) | ||
| stop("No Crossref Data") | ||
| excel_spreadsheet <- list(`Übersicht` = .md$cr_overview, | ||
| `CC-Lizenzen` = .md$cc_license_check) | ||
| if (!is.null(.md$cr_tdm)) { | ||
| excel_spreadsheet[["TDM"]] <- .md$tdm | ||
| } | ||
| if (!is.null(.md$cr_funder)) { | ||
| excel_spreadsheet[["Förderinformationen"]] <- .md$funder_info | ||
| } | ||
| if (!length(tolower(dois) %in% tolower(.md$cr_overview$doi)) > 0) | ||
| excel_spreadsheet[["Nicht-indexierte DOIs"]] <- tibble::tibble( | ||
| mutate(missing_dois = !tolower(dois) %in% tolower(.md$cr_overview$doi)) | ||
| ) |
There was a problem hiding this comment.
I think this might be a bit hard to check/maintain because:
- the function makes unexplicated/untested assumptions about the inner structure of
.md(i.e. if.md$funder_infowere renamed, it would break) - the conditionals aren't very expressive; we don't really mean that
!is.null()is a very special case, we just generally mean that empty sheets shouldn't be included - the calculation of missing dois doesn't really belong in here.
I'll refactor this to show what I mean.
Hope that makes sense.
R/metrics_cc.R
Outdated
| #' | ||
| #' @inheritParams metrics_overview | ||
| #' | ||
| #' @importFrom dplyr count mutate rename |
There was a problem hiding this comment.
these are generally not necessary when using dplyr::mutate().
I think @importFrom statements in the roxygen blocks for individual functions is also a bit confusing/misleading, because the resulting NAMESPACE directives actually affect the entire package.
So, I think when it comes to using functions from other packages, there's basically two scenarios:
- If a lot of our functions use, say,
pkg::foo(), then we should declare@importFrom pkg fooonce inmetacheck.R(conventionally used for that purpose) - If only one/few of our functions use, say
pkg::bar(), then should simply always usepkg::bar(), and never@importFrom.
For dplyr etc., I think it's 1. We're using that all over the place, and I have @import it in #69, which I'll merge in here.
@importing from other packages, there
There was a problem hiding this comment.
Some style guides are discouraging use of the @import tag. https://google.github.io/styleguide/Rguide.html#qualifying-namespaces , but can follow the above scenarios.
R/metrics_cc.R
Outdated
| cc_metrics <- function(.md = NULL, .gt = TRUE, .color = "#F3A9BB") { | ||
| if(is.null(.md) || !"cc_license_check" %in% names(.md)) | ||
| stop("No CC compliance data provided, get data using cr_compliance_overview()") | ||
| else | ||
| out <- .md$cc_license_check %>% | ||
| dplyr::count(cc_norm, name = "value") %>% | ||
| dplyr::mutate(prop = value /sum(value) * 100) %>% | ||
| dplyr::rename(name = cc_norm) | ||
| if(.gt == FALSE) | ||
| out | ||
| else | ||
| ind_table_to_gt(out, prop = prop, .color = .color) | ||
| } | ||
|
|
||
| #' CC compliance metrics overview | ||
| #' | ||
| #' Presents the result of the CC compliance check (absolute and relative) | ||
| #' | ||
| #' @seealso [license_check()] | ||
| #' | ||
| #' @inheritParams metrics_overview | ||
| #' | ||
| #' @importFrom dplyr count mutate rename | ||
| #' | ||
| #' @export | ||
| cc_compliance_metrics <- function(.md = NULL, .gt = TRUE, .color = "#A0A5A9") { | ||
| if(is.null(.md) || !"cc_license_check" %in% names(.md)) | ||
| stop("No compliance overview data provided, get data using cr_compliance_overview()") | ||
| else | ||
| out <- .md$cc_license_check %>% | ||
| dplyr::count(check_result, name = "value") %>% | ||
| dplyr::mutate(prop = value /sum(value) * 100) %>% | ||
| dplyr::rename(name = check_result) | ||
| if(.gt == FALSE) | ||
| out | ||
| else | ||
| ind_table_to_gt(out, prop = prop, .color = .color) | ||
| } |
There was a problem hiding this comment.
there seems to be some duplication here the funs are not type (?) / length-stable.
will probably address that later.
tracking as draft until it's ready