search

Bioconductor / BiocCheck

http://bioconductor.org/packages/BiocCheck
8 stars 26 forks source link

feat: exclude re-exported functions from checkExportsAreDocumented()… #187

Closed gladkia closed 1 year ago

gladkia commented 1 year ago

The goal of this PR is to exclude the man page for re-exported functions from checkExportsAreDocumented() check as it's not possible to add runnable examples for the re-exported functions.

If you are submitting a pull request to BiocCheck please follow the instructions. The presentation includes steps for forking, creating a working branch, and useful related information.

For a successful merge, the following steps are required:

List a reviewer in the Pull Request (either @LiNk-NY, @lshep, @mtmorgan). Reviewers will make sure to Comment, Approve or Request changes. @LiNk-NY

We highly recommend the use of the Bioconductor devel docker image described on the Bioconductor website.

If you have any questions, please get in touch with the Bioconductor core team on the Bioconductor Community Slack.

gladkia commented 1 year ago

@LiNk-NY thanks in advance for having a look :).

LiNk-NY commented 1 year ago

Hi @gladkia Thanks for the PR. I don't see a common use case for this and the explanation is not quite accurate. You can still use @return above the @export tag. Maintainers have multiple options, either to accept the warning, create a dedicated documentation page, or add a @return tag as above. FWIW Re-exports of %>% are no longer needed after R 4.1.0. Best, Marcel

gladkia commented 1 year ago

HI @LiNk-NY,

  1. Sorry for lack of the details explanation in the initial version of the PR.

The purpose of this PR is to support the cases when one has an umbrella package with some re-exported functions. I've added an example 'testpkg3' that mimics the umbrella package (with the selected functions from 'stats' and 'utils' packages).

I've also added some unit tests to show how the changes will affect BiocCheck() for packages with re-exports.

  1. Some links/comments related to this PR
LiNk-NY commented 1 year ago

Hi @gladkia

It looks like they should use the package in the Depends field rather than trying to re-export a good number of functions.

Given that only two packages (linked above) use this re-export mechanism it would not be too much to ask for them to add a @return / @examples tag in their documentation or create a dedicated documentation page. I would rather not make a special case for man/reexports.Rd.

Best regards, Marcel

gladkia commented 1 year ago

Hi @LiNk-NY ,

Thanks for the quick answer and great suggestions! Please find my comments below.

It looks like they should use the package in the Depends field rather than trying to re-export a good number of functions.

Yes. That's a nice workaround. Unfortunately, in some cases, this solution will strongly affect performance (using Depends for the packages with a large number of functions). Thus a re-export mechanism in some cases might be a really good option, pretty performant and elegant. You are able to select only the key exported functions within your umbrella package (e.g. like here: https://github.com/tidyverse/dplyr/blob/29307bf661a8c52b892c28afa531b38461aababb/R/reexport-tibble.R).

Given that only two packages (linked above) use this re-export mechanism it would not be too much to ask for them to add a @return / @examples tag in their documentation or create a dedicated documentation page. I would rather not make a special case for man/reexports.Rd.

Yes, great suggestion. Unfortunately, it's not possible to add @examples tag in the re-exported function if you want to keep your documentation up-to-date with roxygen2. The roxygen2 will fail in such a case. Indeed, the issue can be fixed by manually editing reexports.Rd. But then, the maintenance of the package documentation will be much more complex and error-prone.

Best, Arek

gladkia commented 1 year ago

Hi @LiNk-NY,

Can I help in any way to move this PR forward?

Best, Arek

LiNk-NY commented 1 year ago

Yes. That's a nice workaround. Unfortunately, in some cases, this solution will strongly affect performance (using Depends for the packages with a large number of functions). Thus a re-export mechanism in some cases might be a really good option, pretty performant and elegant.

Do you have metrics to show that it is more performant than using the Depends field? Also, 'elegant' is a matter of opinion.

Yes, great suggestion. Unfortunately, it's not possible to add @examples tag in the re-exported function

This is not accurate, you can certainly add @examples to the roxygen block.

Maintaining the added code is not worth the reward given that you have the options I mentioned previously.

Best, Marcel