nafill gains nan argument

[MichaelChirico]

Closes #4020

I imagine this will need a few small changes but here's 90% of it.

Would be nice to do the C equivalent of testF = if (nan_is_na) ISNAN else ISNA outside the loop but I couldn't figure out the proper way to do that.

[jangorecki]

This is not related but could you also remove verbose arg from function API? and fetch it at the beginning of C function, passing to lower level functions as argument already.

[MichaelChirico]

About verbose, will come back to that

[MichaelChirico]

@jangorecki these two tests are broken, shall we set nan=NaN by default for backwards compatibility? or happy to break this?

test(1.21, nafill(z, "locf"), c(NA,Inf,3,4,NaN,NaN,7,8,-Inf,-Inf)/2)
test(1.22, nafill(z, "nocb"), c(Inf,Inf,3,4,NaN,7,7,8,-Inf,NA)/2)

[jangorecki]

Test can be changed, just mention this change in news

[MichaelChirico]

Hmm @mattdowle seems there was an API failure on the covr step, rest of job is OK. Re-trigger Travis should be enough unless there's ongoing outage (seems all is fine now)

[codecov]

Codecov Report

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

@@            Coverage Diff             @@
##           master    #4025      +/-   ##
==========================================
+ Coverage   99.41%   99.41%   +<.01%     
==========================================
  Files          72       72              
  Lines       13756    13769      +13     
==========================================
+ Hits        13675    13688      +13     
  Misses         81       81

Impacted Files	Coverage Δ
src/nafill.c	100% <100%> (ø)	⬆️
R/utils.R	100% <100%> (ø)	⬆️
R/shift.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 8ce5f09...eb42c60. Read the comment docs.

[jangorecki]

And verbose arg? Could we remove it on R level here?

[MichaelChirico]

And verbose arg? Could we remove it on R level here?

Do we have any rule about when verbose should/shouldn't be provided as a function argument? Seems pretty harmless

^ @jangorecki

[jangorecki]

@MichaelChirico no rules about that, here it looks really unneeded, and could be easily migrated to use option.

[MichaelChirico]

OK done the move, please have a look