-
Notifications
You must be signed in to change notification settings - Fork 2
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
Refactor add labels #167
base: main
Are you sure you want to change the base?
Refactor add labels #167
Conversation
@michaelmcd18 This is ready for your review. |
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.
Two non-blocking comments for you to take or leave, but looks good to me.
R/class-yspec.R
Outdated
col_labels <- map_chr(spec,fun) | ||
for(i in seq_along(data)) { | ||
attr(data[[i]],"label") <- col_labels[[i]] | ||
col_labels <- map_chr(spec, fun) |
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.
nit-pick of no consequence: since col_labels
isn't used until after the guard on 446-448, to my eyes it'd be nicer to move it below so it's the line before col_labels <- col_labels[names(col_labels) %in% names(data)]
.
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.
IC; yep, totally agree ... I like doing this in the code. Thanks for picking it up.
R/class-yspec.R
Outdated
#' @param strict if `TRUE`, generate an error when `data` names do not match | ||
#' the `spec` names; otherwise, only data items in common between `data` | ||
#' and `spec` will be labeled. |
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 reading this, I'm not sure if it'd be obvious to me that "match" means identical
rather than setequal
(i.e. that order matters). May be worth explicitly mentioning order in this description.
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.
I clarified the language to include / exclude order. This makes it seem very strict, but the use case for this function is really labeling the data set corresponding to the spec ... formally writing the data set to .xpt
; but now there's a workaround for the fringe cases where we want to label whatever happens to be there.
Relax the requirement that names must be identical between
data
andspec
. The update adds an argument that bypass the requirement for identical names betweendata
andspec
. If no columns are found in common betweendata
andspec
, the data set is returned unlabeled with a warning.Also, some incremental updates to Rd file format are implemented here, limited to the code relevant to this PR.
See #158