-
Notifications
You must be signed in to change notification settings - Fork 76
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
Set names for import_list() #164
Conversation
Exclude .ext from element name
Uses table's 'class' attribute otherwise blank
Appveyor build failed due to a suspected r-devel issue
Seems to build and check fine locally otherwise:
|
Codecov Report
@@ Coverage Diff @@
## master #164 +/- ##
=========================================
Coverage ? 85.42%
=========================================
Files ? 18
Lines ? 1036
Branches ? 0
=========================================
Hits ? 885
Misses ? 151
Partials ? 0
Continue to review full report at Codecov.
|
R/import_list.R
Outdated
@@ -48,6 +48,7 @@ function(file, | |||
setclass <- NULL | |||
} | |||
if (length(file) > 1) { | |||
names(file) <- gsub(paste0("\\.", tools::file_ext(file[1]), "$"), "", file, ignore.case = TRUE) |
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'm not sure I understand what's happening here. Can you explain?
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.
It's what the first comment in this PR is trying to explain but unfortunately the comment didn't attach itself to the precise line of code for clarity...
Also relates to Case 4 in the PR 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've since realised that assuming all files have the same extension somewhat defeats the purpose of ingesting multiple disparate datasets of differing file types at the same time.
file <- c("mtcars.csv", "iris.csvy", "flights.xlsx", "USArrests.csv")
my_list <- import_list(file, rbind = FALSE)
names(my_list)
#> [1] "mtcars" "iris.csvy" "flights.xlsx" "USArrests"
If it's going to strip file extensions it probably makes more sense to lapply(file, tools::file_ext)
and strip each respective one so that we end up with
exts <- paste0("\\.", lapply(file, tools::file_ext), "$")
names(file) <- sapply(seq_along(file), function(x) gsub(exts[x], "", file[x]))
names(file)
#> [1] "mtcars" "iris" "flights" "USArrests"
Also, on second thoughts it also doesn't really seem appropriate to treat zip
files differently so Case 3 should now be the same as Case 4
zip
retains the file names without extension
Implemented in 258a50c
Can you add yourself to |
R/import_list.R
Outdated
if (length(file) > 1) { | ||
names(file) <- gsub(paste0("\\.", tools::file_ext(file[1]), "$"), "", file, ignore.case = TRUE) | ||
names(file) <- strip_exts(file) |
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.
See #164 (comment)
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.
There's a function tools::file_path_sans_ext
that might be useful (possibly in combination with basename()
).
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.
Great call, thanks @HughParsonage.
@leeper ready for merge, I think
I was just looking for the functionality of maintaining sheet names for xls/xlsx files with import_list, and I found this PR. My fingers are crossed that it will be integrated soon. (If I can help with the integration, please let me know.) |
The PR was finished and has been ready to go pending a review. @billdenney you can install this pull request (and any other for that matter) as follows library(devtools)
install_github("leeper/rio", ref = github_pull(164)) |
Thanks for this! |
@leeper, some thoughts on the enhancement you suggested in #162. It may make more sense to keep the logic for cases 3 & 4 the same (see below) and/or do away with case 2 ... ?
PR to add functionality for imported lists to inherit data structure's names according to:
xls
andxlsx
retain the sheet nameshtml
retains the table's "class" attribute if it exists otherwise blankzip
retains the file names with extensionFixes #162