insightsengineering / tlg-catalog

A catalog of Tables, Listings and Graphs (TLGs) created with NEST R packages
https://insightsengineering.github.io/tlg-catalog/
Other
20 stars 8 forks source link

create testable package in subdir #62

Closed pawelru closed 1 year ago

pawelru commented 1 year ago

Implementation details:

As a result:

Known limitations (might be resolved):

Some other ideas (not yet implemented nor declined):

shajoezhu commented 1 year ago

can we use softlinks for description, .rbuildignore files etc

pawelru commented 1 year ago

for description

Unfortunately not. Tried this path already. For book it has Type: Book that blocks any R CMD * as found by Davide.

.rbuildignore etc.

Probably yes but that's not a big overhead to be honest.

shajoezhu commented 1 year ago

for description

Unfortunately not. Tried this path already. For book it has Type: Book that blocks any R CMD * as found by Davide.

.rbuildignore etc.

Probably yes but that's not a big overhead to be honest.

hey @pawelru , I agree, the overhead is not big. but if we keep two copies, we still need to maintain two copies, and this will be duplicated work for maintaining. thinking out loud, if we don't keep a hard copy of DESCRIPTION file, but when running r cmd check on in cicd, use sed "s/" and modify the description file, and create it on the fly. would this work?

pawelru commented 1 year ago

Oh sorry I previously meant not a big overhead for all ignore type of the files. For DESCRIPTION you are definitely right. I share the same concerns. Haven't yet found out a clever way to address that.

pawelru commented 1 year ago

I am thinking now that in order to address two limitations we would have to have a package as a top-level and book as a child or alternatively there will be two top level dirs: package and book. In other words: package cannot be a child of the book - it can be either a parent or a sibling. This would allow to create a single symlink to the whole book directory as opposed to currently multiple book subdirs and subfiles individually.

├── tlg.catalog
│   ├── book
│   │   └── (book files)
│   ├── (package files)
│   ├── tests
│   │   ├── testthat
│   │   │   ├── _book -> symlink to the top level book dir
│   │   │   └── (other test files)

Or:

├── tlg.catalog
│   ├── book
│   │   └── (book files)
│   ├── package
│   │   ├── (package files)
│   │   ├── tests
│   │   │   ├── testthat
│   │   │   │   ├── _book -> a symlink to the top level book dir
│   │   │   │   └── (other test files)

I think I like the latter more - it's less messy. Whether its the first or the second, those two would require some changes in the deployment logic - I hope it's just a single path parameter. Let me experiment with it a little.

pawelru commented 1 year ago

So I have decided to go towards the latter, i.e. separate top-level dir for a package and a book. I think it's getting nicer and nicer. Please have a look and tell me your thoughts on this.

Some few enhancements:

As a result - I have addressed all the limitations I have identified previously.

New challenges:

Melkiades commented 1 year ago

I think this has priority @edelarua @pawelru @cicdguy. It would make reviewing by comparison with other results much faster and more reliable

pawelru commented 1 year ago

Let me see if this can be merged without CI in place (to be delieved later on after merge). Will do that next week

pawelru commented 1 year ago

I think I have found even nicer way to store outputs for testing via custom knitr hook. Right now you have to modify code chunk header as opposed to add a new one afterwards. I think it's much cleaner. The syntax is as follows: {r <label>, <other options>, test = list(<target object name in testing envir> = "<actual variable name in chunk>")}, e.g. {r variant1, test = list(result_v1 = "result")}. As a result, the required changes to the articles are very minimal - please refer to the modified /book/tables/adverse-events/aet04.qmd to see an example.

I have also removed opts_chunk and opts_current as this was not really working. Luckily, chunks header supports R expressions as chunk option (as opposed as option set in the chunk body prefixed by |#). I really don't know why is that. I sticked to the working solution.

Current state should be fully functional.

github-actions[bot] commented 1 year ago

Unit Tests Summary

8 tests   0 :heavy_check_mark:  5s :stopwatch: 1 suites  8 :zzz: 1 files    0 :x:

Results for commit 8a9ac169.

:recycle: This comment has been updated with latest results.

pawelru commented 1 year ago

OK so I think I resolved staged.dependencies issue. We now need to merge appropriate PRs, release(?) and use the newest versions in the check workflow (as opposed to feature branch currently). Right now it keeps failing due to incomplete suggests - we need to have all pkgs that are used in pkg tests during which we render the whole book. Locally it is all passing but I might have different libpaths. I think it's just a matter of a few iterations to complete that.

pawelru commented 1 year ago

Woooho! It fiinally got green. Before we go with it - @cicdguy can you please have a look at the changes in the docs workflow?