Skip to content
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

extend checks for UTC inputs for robustness #4117

Merged
merged 7 commits into from Dec 17, 2019
Merged

extend checks for UTC inputs for robustness #4117

merged 7 commits into from Dec 17, 2019

Conversation

MichaelChirico
Copy link
Member

@MichaelChirico MichaelChirico commented Dec 16, 2019

Closes #4116

You can confirm all these timezones are the same like:

utc_tz = c("Etc/GMT", "Etc/UTC", "GMT", "GMT-0", "GMT+0", "GMT0", "UTC")
T0 = Sys.time() # ditto for .POSIXct(0) and .POSIXct(-1e6)
length(unique(sapply(utc_tz, function(tz) format(T0, tz=tz))))
# [1] 1

@jangorecki jangorecki added the WIP label Dec 16, 2019
Michael Chirico added 2 commits Dec 16, 2019
@MichaelChirico
Copy link
Member Author

@MichaelChirico MichaelChirico commented Dec 16, 2019

Really dunno why we're getting this on Travis, not able to reproduce:

<simpleError in if (tz == "UTC") {    z <- floor(unclass(x)/86400)    attr(z, "tzone") <- NULL    .Date(z)} else as.Date(as.POSIXlt(x, tz = tz)): argument is of length zero>

That snippet comes from base::as.Date.POSIXct and it should be from if (NULL == 'UTC') but the first tracing commit shows there was never a case of getting tz=NULL passed to as.Date.POSIXct

Michael Chirico added 2 commits Dec 16, 2019
@codecov
Copy link

@codecov codecov bot commented Dec 16, 2019

Codecov Report

Merging #4117 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #4117      +/-   ##
=========================================
+ Coverage    99.4%   99.4%   +<.01%     
=========================================
  Files          72      72              
  Lines       13685   13686       +1     
=========================================
+ Hits        13604   13605       +1     
  Misses         81      81
Impacted Files Coverage Δ
R/IDateTime.R 100% <100%> (ø) ⬆️
R/utils.R 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1c1b26...1f3666f. Read the comment docs.

@MichaelChirico
Copy link
Member Author

@MichaelChirico MichaelChirico commented Dec 16, 2019

I find this PR very strange in that the initial commit didn't work on Travis and the current commit does. any ideas?

@MichaelChirico
Copy link
Member Author

@MichaelChirico MichaelChirico commented Dec 16, 2019

difference in code is

if (is.null(tz)) tz=''
as.IDate(as.Date(x, tz = tz, ...))

vs

as.IDate(as.Date(x, tz =  if (is.null(tz)) tz='' else tz, ...))

I can't see how in the first one tz was passed as NULL to as.Date 🤔

@MichaelChirico MichaelChirico removed the WIP label Dec 16, 2019
# check UTC status
is_utc = function(tz) {
# via grep('UTC|GMT', OlsonNames(), value = TRUE)
utc_tz = c("Etc/GMT", "Etc/UTC", "GMT", "GMT-0", "GMT+0", "GMT0", "UTC")
Copy link
Member Author

@MichaelChirico MichaelChirico Dec 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sorted this alphabetically... perhaps we should sort it by expected frequency instead? Does %chin% have a short-out to return once the first match is found?

Copy link
Member

@mattdowle mattdowle Dec 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Good idea. If its LHS is length 1, then it should just loop through the RHS and stop early if and when it is found. Will do. (Btw, sorting the input was good thought but doesn't make any difference as it's an order-n two-pass approach using the now-well- established truelength clobber technique.)

Copy link
Member

@mattdowle mattdowle Dec 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When %chin% does short-circuit as you suggested, then yes this input should be sorted by expected frequency. Good idea.

Copy link
Member

@mattdowle mattdowle Dec 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: #4121

@mattdowle mattdowle added this to the 1.12.9 milestone Dec 16, 2019
@mattdowle mattdowle merged commit 6b44e76 into master Dec 17, 2019
4 checks passed
@mattdowle mattdowle deleted the extend_fasttime branch Dec 17, 2019
@jangorecki jangorecki removed this from the 1.12.11 milestone May 26, 2020
@jangorecki jangorecki added this to the 1.12.9 milestone May 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants