append without losing timezones in strip_pts #413
Conversation
| } else { | ||
| out <- append(out, NA) | ||
| out <- append_keepTZ(out, NA, tz=attr(out, "tzone")) |
There was a problem hiding this comment.
is it not necessary to set the class here too? Just wondering what cases we lose it for, and if this is a safe one
There was a problem hiding this comment.
In my experience, POSIX's and Dates tend to keep their class, but they get attributes stripped off for various reasons. So, my guess is that this is fine.
There was a problem hiding this comment.
That was only when all of "v.vals" was NA - we had to be explicit about what class it should have. Otherwise, it will have the same class as v.vals.
|
@jread-usgs @ldecicco-USGS merge-ready? |
| } else { | ||
| if (any(sapply(list, is.list))){ | ||
| u.list <- unname_c(list[sapply(list, is.list)]) | ||
| if(v %in% names(u.list)) { | ||
| v.vals <- u.list[[v]] | ||
| out <- append(out, v.vals) | ||
| out.class <- ifelse(!all(is.na(v.vals)), class(v.vals), out.class) | ||
| out <- append_keepTZ(out, v.vals, tz=attr(v.vals, 'tzone')) |
There was a problem hiding this comment.
Would there ever be a case where we were going to use some other tz? If not, should it just be moved into the function?:
append_keepTZ <- function(base.vals, append.vals){
tz <- attr(append.vals, "tzone")
vals <- append(base.vals, append.vals)
attr(vals,"tzone") <- tz
return(vals)
}
There was a problem hiding this comment.
Some keep the tz to be from base.vals, and some from append.vals, so I kept it outside. Most need to look at the tzone of the values you are appending because you could be appending to an empty object. However, sometimes you are appending an NA (see L66), and then you need to keep the tzone from the existing object.
|
So, I merged this, then remembered...it's always good to see that the gsplotIntro.html file has at least a minor update (because we show the date). Then we know that all the vignette and (ideally) readme plots look the same. |
|
Except that we don't commit the html file, so how could we tell if there was a difference? |
|
Why not? I do... |
|
Oh, I see @jread-usgs added it to the git ignore...I have no idea why. I consider it a part of the tests. I'm taking it off the git ignore |
|
No, actually he's right. It's ignored from the vignette folder, but not the "proper" location in inst/doc. So, before each PR...you should be doing: and knitting the Readme.Rmd. |
|
just ran it - it did not actually change anything. I think it's because none of our examples have timezones, so they were showing up just fine. |
|
I didn't expect it to change anything...just mentioning that it's probably a good idea to always include it in your (and all of our) pull requests. At least the date up top changed though, right? https://rawgit.com/ldecicco-USGS/gsplot/master/inst/doc/gsplotIntro.html right under the author list, there's the date. |
|
oh yes - that changed. Worth making a new PR for though? |
|
Agree @ldecicco-USGS, since we can't do visual tests, we should bug each other in PR reviews to include the knits |
|
Not worth another PR (I've actually got it in mine now....)...just mentioning for the future. |
Working on #285