add rm_dirs_from_zip_paths fn #25
Conversation
| f <- strsplit(files_in_gtfs, "^(?=/)(?!//)|(?<!^)(?<!^/)/", perl = TRUE) | ||
| lens <- vapply(f, length, integer(1)) | ||
| if (length(unique(lens)) > 1L) { | ||
| files_in_gtfs <- files_in_gtfs[which(lens == median(lens))] |
There was a problem hiding this comment.
Hi @mpadge, thanks for the PR. It looks fine to me, although I'm a bit worried about licensing, since the regex match is copied from {fs}. I don't really have much experience with licensing, so I'm not sure how to proceed here. Should we just explicitly give attribution to this line of code to {fs} authors?
Two other things that raised my attention:
- Perhaps the subsetting condition should be changed to
which(lens == max(lens))? You mentioned that this solution could handle feeds nested at arbitrary depth, but it seems to me that it would fail if it was nested inside many directories, which could change the value of the median length. If we usemax()here we make sure that we're using the "flattest" files. - I've also come across some weird "GTFS" files that were actually a zipped file containing many directories, each one of them containing a GTFS (think of a
gtfs.zipfile that contained two directories,dir1anddir2, and each one contained a list of gtfs tables). In such case, we would potentially have tables with the same name inside the final GTFS object, if, for example, both of these directories contained anagencytable. I know this case is an exception, but perhaps we should handle it in this function? Something along the lines of, if two or more of the splitted path have the same length, and this length is not the max length (i.e. not the "flattest" files), we throw an error saying that two or more directories have been found inside the zip?
Happy to hear your thoughts, and thanks again for submitting this PR. If you agree with the changes I propose but don't have the time to make them please let me know and I'll accept the PR and make them myself.
There was a problem hiding this comment.
PS: I literally wrote this review on the date you sent the PR but didn't realize I had not "finished" the review... 🤦
Sorry about this.
(there was a huge "Pending" label right next to this comment, but I thought it meant there were pending changes, not pending submission lol)
@dhersz Feeds are sometimes nested within directories prior to zipping (and i don't think the specifications actually explicitly forbid this). When that happens, this line also lists the directory as one of the "files"
gtfsio/R/import_gtfs.R
Line 118 in e083a15
That doesn't end in ".txt", and so triggers the warning in the subsequent line. These feeds are nevertheless processed okay, and there is nothing wrong with the code. This PR simply helps to appropriately identify such cases and avoid the warning. It's perhaps not the best way to identify them, so happy to iterate, but allows feeds to be nested at arbitrary depth within the zip archive - let me know if you think of a better way.
I don't have any live examples, but have it on several older versions of the Madrid feed. It seems to have been updated now to the expected flat format, but definitely used to come in a "google-gtfs" directory.