-
Notifications
You must be signed in to change notification settings - Fork 967
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
gshift() of one column returns a vector, not list(vector) #5950
Conversation
@@ -1268,6 +1268,7 @@ SEXP gshift(SEXP x, SEXP nArg, SEXP fillArg, SEXP typeArg) { | |||
copyMostAttrib(x, tmp); // needed for integer64 because without not the correct class of int64 is assigned | |||
} | |||
UNPROTECT(nprotect); | |||
return(ans); | |||
// consistency with plain shift(): "strip" the list in the 1-input case, for convenience | |||
return isVectorAtomic(x) && length(ans) == 1 ? VECTOR_ELT(ans, 0) : ans; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ben-schwen do we actually need the isVectorAtomic()
check in the gshift
case? Is it possible to get a list()
here in gshift?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now no, the only supported types are the six atomic vector types (otherwise we would error before in the macro switch). AFAIR I deactivated the support for list columns because we use coerceAs
for the fill argument and coerceAs
only supports atomic vectors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see also #4586
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave the isVectorAtomic()
check, simpler to keep consistency with "plain" shift.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5950 +/- ##
=======================================
Coverage 97.48% 97.48%
=======================================
Files 80 80
Lines 14859 14859
=======================================
Hits 14486 14486
Misses 373 373 ☔ View full report in Codecov by Sentry. |
still not desired result. |
Apparently, gshift cannot work with subsetting since it always creates vectors of |
Sorry for hijacking the PR, but it should go into patch release and was also #5939 |
thanks; since this PR targets master let's leave the NEWS out. I'll add the NEWS to the copycat PR targeting the patch release. Or maybe we should target the patch instead 🤔 |
Whichever one we target, we just cherry pick the other one. |
The latest commit works for me now. Thanks! |
I think we actually have two distinct bugs on our hands, so I will cherry-pick your fix into a separate PR. |
ca99fad
to
326cefd
Compare
326cefd
to
9e3b0be
Compare
@ben-schwen / @jangorecki any final review here? running up against our CRAN deadline |
LGTM |
Closes #5939
No NEWS since the NEWS item belongs in the patch branch.