Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set up skeleton API package #1

Merged
merged 22 commits into from
Jul 26, 2024
Merged

Set up skeleton API package #1

merged 22 commits into from
Jul 26, 2024

Conversation

pratikunterwegs
Copy link
Contributor

This PR sets up {daedalus.api} with some skeleton API code taken from {odin.api}:

  1. Updates the .lintr file to conform to modern {lintr} and tweak exclusions for;
  2. Updates the GHA workflows;
  3. Updates tests - removed {odin}-specific tests leaving only basic API tests;
  4. Updates the Dockerfile and Rprofile.site to be use P3M.

Some outstanding points/issues:

  1. I'm not having much luck updating to rocker/r-ver:4.4.1 as {R6} and {jsonlite} aren't installing which blocks {porcelain};
  2. There's little to no Roxygen function documentation - is this something that needs to be added?
  3. The Dockerfile still takes mrc-ide/drat as a repo, I can change this once we set up an R-universe;
  4. The R CMD check GH workflow allows NOTEs to pass - this because {daedalus} is imported but not used (this will likely change?)

@pratikunterwegs pratikunterwegs self-assigned this Jul 24, 2024
@pratikunterwegs pratikunterwegs marked this pull request as ready for review July 24, 2024 14:09
Copy link
Contributor

@richfitz richfitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tiny things, then let's get this merged

@@ -0,0 +1,14 @@
# Generated by porcelain: do not edit by hand
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you set up .gitattributes with

R/porcelain.R linguist-generated=true

then this will not render in the diff view by default

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 792b3e7

R/util.R Outdated
as.character(utils::packageVersion(name))
}

uglify <- function(code) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and prepare_code can be dropped I think?

Copy link
Contributor Author

@pratikunterwegs pratikunterwegs Jul 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in 9e12b17 - wasn't sure whether these would be needed later so kept them initially

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were for uglifying js code returned from odin, but we removed the implementation in the end as it was hard to get it working as expected so I think I left it in as an empty skeleton and then forgot about it...

docker/Dockerfile Outdated Show resolved Hide resolved
@richfitz
Copy link
Contributor

Your queries:

I'm not having much luck updating to rocker/r-ver:4.4.1 as {R6} and {jsonlite} aren't installing which blocks {porcelain};

this is now fixed 🎉

There's little to no Roxygen function documentation - is this something that needs to be added?

that's fine, roxygen is just a tool for us to generate the interface for now

The Dockerfile still takes mrc-ide/drat as a repo, I can change this once we set up an R-universe;

this can be changed now, and you can also set up a jameel-institute one to pull daedalus itself in (now or later as you prefer)

The R CMD check GH workflow allows NOTEs to pass - this because {daedalus} is imported but not used (this will likely change?)

I never block on notes, because of notes like the "more than 5mb of compiled C++" one that blocks just about everything using Rcpp/cpp11

@pratikunterwegs
Copy link
Contributor Author

pratikunterwegs commented Jul 26, 2024

this can be changed now, and you can also set up a jameel-institute one to pull daedalus itself in (now or later as you prefer)

I guess this can be left for now as {daedalus} is pulled direct from GH anyway as the remote is listed.

I think the more recent commits fix the other issues.

@richfitz
Copy link
Contributor

I guess this can be left for now as {daedalus} is pulled direct from GH anyway as the remote is listed.

this is fine until we hit rate limits, as we can't get the token safely into the builder. if/once we hit that the fix is easy and by then we'll probably have the universe set up

Copy link
Contributor

@richfitz richfitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fabulous, thanks

@pratikunterwegs
Copy link
Contributor Author

pratikunterwegs commented Jul 26, 2024

this is fine until we hit rate limits, as we can't get the token safely into the builder. if/once we hit that the fix is easy and by then we'll probably have the universe set up

I set it up yesterday/the day before - works alright. I can add the R-universe to the Dockerfile.

@pratikunterwegs pratikunterwegs merged commit 1714a63 into main Jul 26, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants