search

tidyverse / vroom

Fast reading of delimited files
https://vroom.r-lib.org
Other
621 stars 60 forks source link

Don't inherit docs from readr #463

Closed sbearrows closed 2 years ago

sbearrows commented 2 years ago

Closes #461

Moving over docs from readr to vroom so that the primary docs live here which we think makes more sense.

Note: this would also fix part of https://github.com/tidyverse/readr/issues/1421 (removing mention of list() with col_select) but we'd still need to add examples of col_select()

jennybc commented 2 years ago

I need to you do some narration of this task, so I don't have to reverse engineer the stuff below while reviewing. There's a lot to be checked.

Along these lines:

We're tackling the vroom() function. Currently, n arguments are documented via inheritance from readr::some_fcn() and m arguments are documented here in vroom. Of those, k are only relevant to vroom() and j are relevant to both and are documented in both places. Maybe list the arguments in those categories?

I suggest backing up and starting this PR over and probably working with 1 commit per argument. I don't think we're going to have the appetite for doing this wholesale and it's not the best use of our time before release.

From seeing the first commit here, I suggest we prioritize arguments that only work in vroom / readr 2e, like col_select. It seems clear the primary docs for such an argument should live in vroom. I don't know how many arguments that applies to (which is what the overview of the task would tell us). Let's find and fix these and then re-assess if this task feels worth it relative to everything else that has to happen.

If we decide to keep going, then we'd look at arguments that are relevant and documented in both packages, but there's a big difference in the quality of readr's and vroom's docs. Usually, but not always, the vroom docs are less well developed. Then address, probably by writing new parameter docs by combining the best of the readr and vroom text. Example: https://github.com/tidyverse/vroom/commit/ea2358d0c53ae8895e3d825a074c909d30abd3de.Then we'd put this new text in vroom and inherit that from readr.

Watch out for: arguments where behaviour differs in readr 1e and readr 2e / vroom. We can't harmonize willy-nilly if there's a reason the docs differ. This is why I don't think we should do this for all parameters now.

jennybc commented 2 years ago

I'm feeling more optimistic about the wholesale update, so let's proceed with that, but with more narration, so it's easier to feel confident about the changes.