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

Closes #1973; lazyeval edge case of IDateTime caused an error in as.D… #2062

Merged
merged 3 commits into from
May 11, 2017
Merged

Closes #1973; lazyeval edge case of IDateTime caused an error in as.D… #2062

merged 3 commits into from
May 11, 2017

Conversation

MichaelChirico
Copy link
Member

…ate.POSIXct

See discussion @ #1973. Quite a strange bug, not sure I fully wrap my head around how lazyeval is working here... may be that there's a better solution?

@codecov-io
Copy link

codecov-io commented Mar 17, 2017

Codecov Report

Merging #2062 into master will decrease coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2062      +/-   ##
==========================================
- Coverage   90.69%   90.58%   -0.11%     
==========================================
  Files          59       58       -1     
  Lines       11431    11012     -419     
==========================================
- Hits        10367     9975     -392     
+ Misses       1064     1037      -27
Impacted Files Coverage Δ
R/IDateTime.R 77.16% <100%> (+0.74%) ⬆️
R/fread.R 90.47% <0%> (-4.19%) ⬇️
R/utils.R 76.19% <0%> (-3.81%) ⬇️
src/fsort.c 72.72% <0%> (-1.22%) ⬇️
src/openmp-utils.c 95% <0%> (-0.24%) ⬇️
R/onLoad.R 0% <0%> (ø) ⬆️
src/wrappers.c 95.08% <0%> (ø) ⬆️
R/fwrite.R 97.29% <0%> (ø) ⬆️
src/init.c 93.1% <0%> (ø) ⬆️
R/data.table.R 97.44% <0%> (ø) ⬆️
... and 5 more

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 2787644...0e68a72. Read the comment docs.

@mattdowle mattdowle merged commit 05f62b8 into Rdatatable:master May 11, 2017
@MichaelChirico MichaelChirico deleted the idatetime_tz branch May 12, 2017 01:48
# IDateTime error when tzone is NULL, #1973
x = as.POSIXct('2017-03-17')
attr(x, 'tzone') = NULL
test(1765, print(IDateTime(x)), output=".*idate.*itime.*1: 2017-03-17 00:00:00")
Copy link
Member

@jangorecki jangorecki Jun 25, 2017

Choose a reason for hiding this comment

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

@MichaelChirico
This test on my machine produces:

  Test 1765 didn't produce correct output:
  > print(IDateTime(x)) 
  Expected: '.*idate.*itime.*1: 2017-03-17 00:00:00'
  Observed: '        idate    itime1: 2017-03-16 00:00:00'

If timezone set to "Poland". If timezone set to "GB" test pass fine.
Poland is +1 hour vs GB so it shifts the date already. It should be fixed so users from various timezones can run tests.

> sessionInfo()
R version 3.4.0 (2017-04-21)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Ubuntu 16.04.2 LTS

Matrix products: default
BLAS: /usr/lib/libblas/libblas.so.3.6.0
LAPACK: /usr/lib/lapack/liblapack.so.3.6.0

locale:
 [1] LC_CTYPE=en_GB.UTF-8       LC_NUMERIC=C              
 [3] LC_TIME=en_GB.UTF-8        LC_COLLATE=en_GB.UTF-8    
 [5] LC_MONETARY=en_GB.UTF-8    LC_MESSAGES=en_GB.UTF-8   
 [7] LC_PAPER=en_GB.UTF-8       LC_NAME=C                 
 [9] LC_ADDRESS=C               LC_TELEPHONE=C            
[11] LC_MEASUREMENT=en_GB.UTF-8 LC_IDENTIFICATION=C       

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

loaded via a namespace (and not attached):
[1] compiler_3.4.0

To reproduce set timezone on your workstation to Poland and run R check.

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.

4 participants