request for 'keep_qaqcstatus' argument in import_local()

[swmpkim]

I have a new need to filter data based on columns I've basically ignored since the beginning of time: 'Historical' and 'ProvisionalPlus'. The column names are the same in wq, nut, and met files. Could an argument be inserted into import_local() to keep them, without messing up anything else the package does?

I have it working interactively with the modifications below, though I don't know if it's the cleanest way to do it, or if you want to build in any checks, or.... something. It's just the arguments and then lines 193-199 in the original function.

import_local <- function(path, station_code, trace = FALSE, collMethd = c('1', '2'), keep_qaqcstatus = FALSE)    

# convert output to data frame
    # retain only relevant columns
    if(keep_qaqcstatus == TRUE){
        out <- data.frame(
            datetimestamp = out$datetimestamp,
            historical = out$historical,
            provisionalplus = out$provisionalplus,
            out[, names(out) %in% nms], 
            row.names = seq(1, nrow(out))
        )
    } else {
        out <- data.frame(
            datetimestamp = out$datetimestamp,
            out[, names(out) %in% nms], 
            row.names = seq(1, nrow(out))
        )
    }

Full disclosure, there's a new-ish qaqc column in recent years called 'FRecord' and that is probably also worth figuring out how to keep (it's not specific to a single parameter, but goes with the whole datetimestamp row). There are no numeric flags in that column, only the letter codes, but I'm worried the 'f' means it might affect things like qaqcchk() and qaqc(). Anyway, this is in my head too if you'd rather think about it alongside the qaqc status columns.

Let me know what you think, if you need any more info, etc. Thanks!

[fawda123]

Hey @swmpkim, this shouldn't be difficult to add. I'll try to have a look later today.

[fawda123]

@swmpkim this is added now, but I haven't done a full check to see if it breaks downstream functions. This is my fault for not including any unit tests in the package.... it was originally developed before testthat, but honestly just laziness for not including after the fact. I'll start working on that.

[swmpkim]

Awesome; thanks so much @fawda123 ! I suspect it won't cause problems for several reasons: default is false so it won't change anything for people who don't consciously call it; people who do keep the columns should know enough not to do anything with them that would cause issues (e.g. I only care about them along with the other qaqc columns, not while trying to do additional things like aggregate or plot the data, and wouldn't keep them if I do the latter); other functions in the package error (informatively) if you try to do something you shouldn't.

Having said that, I do enjoy finding creative ways to break things, so I'll play around as I have time. Happy to try to help develop unit tests, with the big caveat that I've never done it outside a workshop context before so I might need some hand-holding. It's a skill I should work on though for sure.