fix first-last xts dispatch #4053

[jangorecki]

closes #4053

[jangorecki]

this tail(df) was actually a mistake in old unit test, it was now amended to last(df) as it should be in the first place

[codecov]

Codecov Report

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

@@            Coverage Diff             @@
##           master    #4065      +/-   ##
==========================================
+ Coverage    99.4%   99.41%   +<.01%     
==========================================
  Files          72       72              
  Lines       13728    13756      +28     
==========================================
+ Hits        13647    13675      +28     
  Misses         81       81

Impacted Files	Coverage Δ
R/last.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 7b493fd...37a36b3. Read the comment docs.

[mattdowle]

Is there a smaller solution than +165 -35 ?

[MichaelChirico]

net 101 of that is in tests though right?

[jangorecki]

I don't think we can rework the logic to save some lines. As @MichaelChirico noticed, a lot of those lines are tests. Verbosity takes some extra lines too, but it is useful for testing.
Another thing I got in mind was an extra argument last = function(x, n, ..., fast=FALSE) when setting fast=TRUE we would ignore xts checks and remove all overheads that are here for compatibility. For speed, it is now better to use x[length(x)] on a vector, and x[nrow(x),] on DT. So first and last will always suffer from an overhead, unless we give user control to ignore compatibility.
Also there could be extra argument, lets say fill=FALSE, when true then result would be always length 1, so for a 0 length that would be an NA. But those basically are enchancement, so wanted to keep this PR only on fixing xts.

[mattdowle]

But why are so many new tests needed? It's just first and last! Out of all the complexity we have, these are really simple functions. How has it become so complicated that so many tests are needed? We were aware of the conflict and differences of our first/last to xts' first/last before, and had tests for that. What's gone so wrong that needs all this change for something so simple?!

[jangorecki]

These tests are newly added, they are likely to overlap to existing test, but they test for verbose output, so they are more accurate and will detect more changes in future. Functions are simple but integration to xts complicates a lot.

[jangorecki]

If we want to have simple functions then as I proposed above we could provide an option to bypass checks for xts compatibility. IMO it make sense to make a new PR for that and in this one only fix regression in method dispatch.

[mattdowle]

@jangorecki seems to be just failing one GLCI job (dev-cran-lin) with:
2 errors out of 9684. Search tests/tests.Rraw for test numbers: 2108.82, 2108.92
Will leave to you for follow-up PR.