search

therneau / survival

Survival package for R
389 stars 105 forks source link

Fix partial matching in the code base #252

Closed MichaelChirico closed 5 months ago

MichaelChirico commented 5 months ago

WIP. Surfacing issues under all of the warnPartialMatch* options() that pop out under {survival}'s own tests.

therneau commented 5 months ago

I'm new to merging a pull, and ended up accepting it before checking it. On later review, all the catches appear to be valid. Howeve, I agree with "dotting the i's and crossing the t's" for the source code, for the test suite and the vignettes not so much --- there is now a tradeoff between simplicity and formality. Some of these may revert. (Some of these made me wince, i.e., did I really call an argument 'times' rather than 'time'? It looks like I did.)

MichaelChirico commented 5 months ago

I would be ok skipping the partial matches in vignettes (I only started catching those by looking for lints which searches the whole repo, vs running tests & finding failures), but would suggest reconsidering for tests.

Part of this is coming from my suggestion that R turns on these stricter options for R CMD check generally (which I saw you replied to) -- ultimately that means the test suite would need to pass.

I also think it's good to write code more formally in tests in principle.

PS sorry, I forgot to mark this PR as WIP -- there is still more to be done. I'll file a follow-up PR. I believe we need to re-generate the .Rout goldens for the tests, for example.