MRCIEU / ieugwasr

R interface to the IEU GWAS database API
https://mrcieu.github.io/ieugwasr/
Other
67 stars 23 forks source link

Preparing for CRAN #56

Closed ritarasteiro closed 3 months ago

ritarasteiro commented 3 months ago

Prepare this package for CRAN.

remlapmot commented 3 months ago

I recommend avoiding using drat if we can. As you said you could take TwoSampleMR out of gwasglue2, then get gwasglue2 on CRAN before this which would leave this without any Remotes.

Also there is no need to export the pipe in this, but we still need to import it, to get that code you can run usethis::use_pipe(export = FALSE).

remlapmot commented 3 months ago

I recommend deleting the version check - my guess is that CRAN would not allow this (there are several other IEU packages this needs to be removed from also) https://github.com/MRCIEU/ieugwasr/blob/8762a2568f10c21922788fbf7155c3df84d26930/R/zzz.R#L30-L45

explodecomputer commented 3 months ago

Hey @remlapmot @ritarasteiro I did some work on this and submitted 38dd7b20d1933845590325af60807d96672ee88a to CRAN already actually. We discussed that we submit this to cran before merging in gwasglue2 so that there would be no remotes. I guess I'll hear back in a few days as to whether it is accepted. In the meantime I'm going to try to get TwoSampleMR to be free of remotes also. This will position gwasglue2 to be ready for CRAN without needing remotes. However, now that this pull request has been merged into master I'm not quite sure what to do. Perhaps just remove the gwasglue2 functionality temporarily if we need to resubmit to CRAN? We can cross that bridge when we get there.

explodecomputer commented 3 months ago

The other thing is that it was at 0.1.8 but this merge has moved it back to 0.1.7 I think. But at some point agree that we should just move to 0.2.0

remlapmot commented 3 months ago

Since this will be a new package on CRAN it will go through review by an experienced CRAN maintainer.

Out of my CRAN packages only 1 was accepted on first submission without any changes. Hence I guess it's unlikely that this will be accepted straight away, so we can sort things for the likely resubmission.

Also, could you share the review with us when you get it from CRAN.

explodecomputer commented 3 months ago

As you predicted! Here is the review:

Please omit the redundant "in R" at the start of your title and description.

Please add a web reference for the API in the form <https:.....> to the description of the DESCRIPTION file with no space after 'https:' and angle brackets for auto-linking.

Please add \value to .Rd files regarding exported methods and explain the functions results in the documentation. Please write about the structure of the output (class) and also what the output means. (If a function does not return a value, please document that too, e.g. \value{No return value, called for side effects} or similar)

-> Warning: Missing Rd-tags: cor.Rd: \value logging_info.Rd: \value pipe.Rd: \value revoke_access_token.Rd: \value select_api.Rd: \value

Please write TRUE and FALSE instead of T and F.

ritarasteiro commented 3 months ago

The other thing is that it was at 0.1.8 but this merge has moved it back to 0.1.7 I think. But at some point agree that we should just move to 0.2.0

@explodecomputer Yes, it was me. I did not know you already had submitted to CRAN. Maybe we can create a dev branch with the version now on main, and revert the main to the previous.

ritarasteiro commented 3 months ago

This will position gwasglue2 to be ready for CRAN without needing remotes.

gwasglue2, when using the yaml constructors and gwasglue() uses ieugwasr and gwasvcf. They are not on remotes, but they should be there, as there will be warnings in check().

ritarasteiro commented 3 months ago

Maybe on a first submission of gwasglue2, we do not have the yaml constructors and gwasglue(). These will stay just on the github dev version. gwasvcf should also be submitted to Bioconductor without the option of outputting gwasglue2 objects.

That way, just after all the packages are on CRAN or Bioconductor, we update them to have these functionalities.

ritarasteiro commented 3 months ago

On ieugwasr, the only functions that call gwasglue2 are associations() and tophits(). We just need to remove the if conditions see here and replace them with return(out).

There is also the ld_scores.R files that can stay, or added in a next update on CRAN. If it is not submitted now, the digest package should be removed from DESCRIPTION.

ritarasteiro commented 3 months ago

Also there is no need to export the pipe in this, but we still need to import it, to get that code you can run usethis::use_pipe(export = FALSE).

@remlapmot The package dplyr is on Imports and the pipe will be automatically imported. Do we need magrittr and utils-pipe.R?