Coerce 01

[mattdowle]

Closes #996
(Closes #3915 too; slightly related regarding NA factor level)
Motivation in the news item.
The complex code removed needed work anyway as it was allocating within a truelength-clobber region. Clearly it's not ideal to making a change as big as this at this stage. On the other hand, it clears up a lot of wrinkles, hopefully.
Several potential coerces removed; e.g. when assigning factor to factor it was coercing the source to character which has gone away now. Plus some other similar todo done.Still some new todo's inline but it's passing locally. Will run it past revdeps to see the impact.
Interested in first thoughts on this approach.

[codecov]

Codecov Report

Merging #3909 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #3909   +/-   ##
=======================================
  Coverage   99.43%   99.43%           
=======================================
  Files          72       72           
  Lines       13548    13548           
=======================================
  Hits        13471    13471           
  Misses         77       77

Impacted Files	Coverage Δ
src/init.c	100% <ø> (ø)	⬆️
R/fread.R	100% <100%> (ø)	⬆️
src/rbindlist.c	100% <100%> (ø)	⬆️
src/assign.c	100% <100%> (ø)	⬆️
R/test.data.table.R	100% <100%> (ø)	⬆️
src/dogroups.c	96.96% <100%> (ø)	⬆️
R/utils.R	100% <100%> (ø)	⬆️
src/utils.c	100% <100%> (ø)	⬆️
R/bmerge.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 5ab1884...7f34680. Read the comment docs.

[MichaelChirico]

i understand the overall gist. makes sense.

is there a lot of overlap with what's done here & what's done for rbindlist? makes sense to share logic then?

[jangorecki]

Have go through all the logic change, some minor feedback left.
Looking forward for revdeps status.

[mattdowle]

Thanks for the great reviews and comments. Seems like a cautious and tentative thumbs up then, depending on revdep results and addressing comments.
5 of 740 packages affected under this PR: bigreadr hutils restatapi VIM wyz.code.testthat
restapi and wyz.code.testthat are newly in error status on CRAN with v1.12.2
bigreadr fleeting resource/download problem; works on rerun
2 of 740 packages affected under this PR: hutils VIM
At least one seems correct: "Cannot assign 0.571429 to a logical column.". Working through them...

[mattdowle]

Now passes the 2 revdeps that were affected (hutils and VIM). Will do another full rerun after getting full coverage.
Condensed logic using macro. This gets the salient coerce parts next to each other for easier study. Overall it's now less lines: diff was +368 - 276 yesterday and now with the macro is +366 - 398.

[jangorecki]

some new comments
ideally, for 1.13.0 we could try to take out coercion from assign and make it re-usable. There are multiple places where efficient and class-aware coercion would be useful. Some initial work on that is in "coerceClass" PR #3765 but your code is far more superior (filled #3913).

[mattdowle]

Good: the vertical alignment and formatting for coverage is doing its job:

(body has built-in break)

[mattdowle]

More re-review needed please. Merging to run it through stronger tests in GLCI and so I can move on to the remaining issues in this milestone. Can still add comments in the closed PR and/or open follow-up PRs. Will do another full revdep rerun too.

[jangorecki]

As of now I see multiple places that could be extended with newlines for C codecov.

[jangorecki]

we have some strange error not related to this PR

Pulling docker image registry.gitlab.com/jangorecki/dockerfiles/r-devel ...
ERROR: Preparation failed: Error response from daemon: received unexpected HTTP status: 500 Internal Server Error (executor_docker.go:188:0s)

only for R-devel linux image.
I will rebuilt the image.

[jangorecki]

Seems to be working fine now.