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

fixes R-devel foverlaps failures due to c.POSIXct change #4428

Merged
merged 2 commits into from
May 19, 2020

Conversation

jangorecki
Copy link
Member

fixes

  Test 1390.3 produced 1 errors but expected 0
  Expected:
  Observed: 'origin' must be supplied
  Test 1390.4 produced 1 errors but expected 0
  Expected:
  Observed: 'origin' must be supplied
  Test 1390.5 produced 1 errors but expected 0
  Expected:
  Observed: 'origin' must be supplied
  Test 1985.1 produced 1 errors but expected 0
  Expected:
  Observed: 'origin' must be supplied
  Test 1993.1 produced 1 errors but expected 0
  Expected:
  Observed: 'origin' must be supplied

@codecov
Copy link

codecov bot commented May 4, 2020

Codecov Report

Merging #4428 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4428      +/-   ##
==========================================
- Coverage   99.61%   99.60%   -0.01%     
==========================================
  Files          72       72              
  Lines       13917    13918       +1     
==========================================
  Hits        13863    13863              
- Misses         54       55       +1     
Impacted Files Coverage Δ
R/foverlaps.R 100.00% <100.00%> (ø)
src/assign.c 99.84% <0.00%> (-0.16%) ⬇️

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 dd7609e...864d9eb. Read the comment docs.

@jangorecki jangorecki changed the title fixes R-devel failures due to c.POSIXct change fixes R-devel foverlaps failures due to c.POSIXct change May 4, 2020
@jangorecki jangorecki mentioned this pull request May 6, 2020
@mattdowle
Copy link
Member

mattdowle commented May 19, 2020

Great. Needs news item. For example, when this change in R-devel is released, and users start getting this error message in data.table <= 1.12.8, then if they search for the error, there's a chance they'll find the news item and find out they need to upgrade data.table.

@jangorecki
Copy link
Member Author

@mattdowle done

@mattdowle
Copy link
Member

This missed coverage line is odd, doesn't seem related to this PR. Will let it go. If it's truly now uncovered then master coverage will show it and it won't get forgotten.
image

@mattdowle mattdowle merged commit f477fac into master May 19, 2020
@mattdowle mattdowle deleted the foverlaps-fix-r-devel branch May 19, 2020 22:50
@mattdowle mattdowle added this to the 1.12.9 milestone May 19, 2020
@jangorecki
Copy link
Member Author

@mattdowle see #4424

@jangorecki jangorecki modified the milestones: 1.12.11, 1.12.9 May 26, 2020
@eddelbuettel
Copy link
Contributor

Are we sure this only affects r-devel? It seems to also affect R-patches if I am reading the CRAN results page for data.table correctly (but not R-release).

It affects us in Debian as the release data.table fails its tests with R 4.0.0, effectively block transition of r-base 4.0.0 (and now r-base 4.0.1) from unstable into testing. See #962497.

@jangorecki
Copy link
Member Author

Quite possibly R-patched as well. It is actually surprising that base R decided to make a breaking change like this without first giving a warning for at least one version.
We are planning to publish to CRAN soon, so hopefully it will not last for long.

@eddelbuettel
Copy link
Contributor

And the actual fix is a one-liner to maybe the Debian maintainer for data.table can carry that over. I probably would if it were my package (though this speculating as there may be other things too betweeb R 4.0.0 and data.table 0.12.8).

I think the change to c.POSIXct was pretty late-breaking as well. Oh well. Spilled milk now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants