-
Notifications
You must be signed in to change notification settings - Fork 1
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
Pull table notes from tex glossary files and yaml #326
Conversation
@kylebaron I have a couple of questions about what this is doing (and if it's what we want). Above you say:
So it's the information in the {label}{definition} that will go in the footer right? Can these functions handle cases where you might was the {abbreviation}{definition} combo? Maybe as an optional extra. For example, the CV% that you may have in the table usually uses CVP in the glossary:
I just had a similar case on a project where tables used FAPα but we couldn't use the greek letter in the glossary label, so the label was FAPa and the abbreviation included the greek letter It would be nice to be able to specify whether label or abbreviation get used. Different question, will glossary entries like this (below) mess with your current code?
|
Hey @KatherineKayMRG - Good points. I think we want to refer to to the
I think the code handles this as it is. |
I'm going to refactor this ... will be more complicated but I think there is a need. |
@kyleam - I think I addressed everything here, adding tests where I missed badly on some of the implementation. But let me know if I didn't get one of the comments right or if I just overlooked anything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a user perspective, this looks good to me and I'm looking forward to using it. No more questions from me. I'll leave the code review to Kyle M.
Thanks, @KatherineKayMRG. It sounds like @timwaterhouse is going to give this a spin on upcoming project and we can tweak and adjust some things from there. |
@kylebaron - that project of @timwaterhouse's is the one I was mentioning on slack. I've been reworking their reports to use a shared glossary file and I'm looking forward to trying out this functionality on that project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates. I have one follow-up comment about a case where print.glossary
still errors, but otherwise this looks ready to go.
(I see there are merge conflicts, but those are just in the generated coverage reports.)
return(invisible(NULL)) | ||
} | ||
label <- names(x) | ||
def <- map_chr(x, "definition") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The case I mentioned here will still fail:
as_glossary(ss = "steady state")[2]
#> Error in `map_chr()` at pmtables/R/glossary.R:311:3:
#> ℹ In index: 1.
#> Caused by error:
#> ! Result must be length 1, not 0.
#> Run `rlang::last_trace()` to see where the error occurred
The length guard won't catch that because of this named-list behavior:
x <- list(foo = "foo")
x[2]
#> $<NA>
#> NULL
length(x[2])
#> [1] 1
How about this guard instead?
diff --git a/R/glossary.R b/R/glossary.R
index 76fc618..3016dc9 100644
--- a/R/glossary.R
+++ b/R/glossary.R
@@ -303,8 +303,8 @@ print.glossary_entry <- function(x, ...) {
#' @export
print.glossary <- function(x, ...) {
- if(!length(x)) {
- cat("No glossary entries found.")
+ if(is.null(unlist(x))) {
+ cat("No glossary entries found.\n")
return(invisible(NULL))
}
label <- names(x)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks; that looks like it works. Added tests for zero-length and mis-indexed objects.
Summary
The PR adds functionality to create table notes from a tex glossary file.
An entry in a glossary file looks like this
\newacronym{label}{abbreviation}{definition}
The basic idea is to read and parse the glossary file to create a string of the form
label: definition
. Likely multiple terms will be selected in which the string will take the formlabel1: definition1; label2: definition2
.The PR also implements ability to read from glossary info in yaml-formatted file.
Objects
glossary
- a list of glossary entries; the names of the list are the glossarylabels
glossary_entry
- a list containing theabbreviation
and thedefinition
Functions
read_glossary()
reads and parses the glossary file; returns a glossary objectglossary_notes()
takes in the glossary file name or a glossary list as well as labels to select and returns a character vector that can be added to a table viast_notes()
st_notes_glo()
takes a glossary list (fromread_glossary()
) and labels to select and adds the notes in a table pipelineas_glossary()
coerce a list to a glossary objectupdate_abbrev()
- you can update the abbreviation for any entry (but can't change the label or the definition)Reprex
Read in a glossary file
We can read from a
.tex
glossary fileor a yaml file
The result is a glossary object
yaml format
The yaml file needs to be written to be read by
yaml_as_df()
; theouter level is the
labels
and inner are theabbreviations
anddefinitions
Create a glossary object
By default, the abbreviation is taken to be the label
Update abbreviation
Work with glossary object
A lot of this is driven by need to potentially combine glossary objects
or look into an object to see what is in there. I would have rather stuck
with a simpler data structure, but it needed to be more complex and I added
this functionality.
Extract and print
Head
Select
Combine
Coerce
data frame
list
Create notes
With a subset (expected most of the time)
With all entries
In a pipeline
Alternatively
Pass in names
Created on 2024-05-23 with reprex v2.0.2