-
Notifications
You must be signed in to change notification settings - Fork 1k
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
setDT generates shallow copy earlier to avoid interfering with attributes of co-bound tables #6551
Conversation
Generated via commit 21b33c8 Download link for the artifact containing the test results: ↓ atime-results.zip
|
Thank you @MichaelChirico. I feel I should say - not only I don't have access to my original fork, but I'm also currently not working with data.table and do not expect to be contributing in the foreseeable future. If you prefer to exclude me from the project members I completely understand. |
No worries @OfekShilon, you had mentioned similar elsewhere (can't see it right away). My comment is as much of a note to self/fellow readers as to you :) Right now we don't have any policy for removing ppl from 'Maintainers', so the easier thing is to leave you in there and hope we'll see you back eventually! Thanks for all your contributions over the years 🙏 |
@ben-schwen do you think there's any test we should add for your 2nd suggested change (running |
Answering my own question, run at 899e49b
|
@ben-schwen based on the test failures I think this approach is the right way to go -- rather than relying on |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6551 +/- ##
=======================================
Coverage 98.62% 98.62%
=======================================
Files 79 79
Lines 14449 14450 +1
=======================================
+ Hits 14250 14251 +1
Misses 199 199 ☔ View full report in Codecov by Sentry. |
Closes #4784. Original author: @OfekShilon.
This is #6470, reborn (again). I tried resolving the merge conflict on that PR but I was rejected trying to push to Ofek's fork:
Just FYI to @OfekShilon -- now that you're a project member, should the need arise, making branches on this repo going forward will avoid such issues.