insightsengineering / teal.modules.general

General Purpose Teal Modules
https://insightsengineering.github.io/teal.modules.general/
Other
9 stars 13 forks source link

Packages listed in `.pre-commit-config.yaml` but no in `DESCRIPTION` #753

Closed m7pr closed 6 months ago

m7pr commented 6 months ago

If you double check the contents of pre-commit-config.yaml you can see that there is more packages listed than there is atucally listed in DESCRIPTION.

packages_in_pre_commit <-
    yaml::read_yaml('.pre-commit-config.yaml')$
    repos[[1]]$hooks[[2]]$additional_dependencies |>
    gsub('insightsengineering/', '', x = _)

packages_in_dependencies <-
  desc::desc_get_deps() |>
    dplyr::filter(type != 'Suggests') |>
    dplyr::pull(package)

setdiff(packages_in_pre_commit, packages_in_dependencies)
> [1] "ggrepel"    "teal.slice"

@pawelru should we revisit contents of pre-commit-config.yaml in all repos and remove extra packages from this file? Is there a way (based on my snippet code) to create an automated run/job to detect such packages and automatically remove/add?

Extra question: why we use insightsengineering/teal.* GitHub versions for some packages, but not for teal and not teal.transform. Should we unify to all released packages, or all GitHub packages? So it's a difference between main branch and the last tag release. We can also put a tag in the .yaml spec to always take the release package, but this is equivalent to the CRAN release in our case.

pawelru commented 6 months ago

I did this reconciliation quite recently so I don't expect big differences. I would say if you spot this - please fix immediately without additional overhead of issue and its estimation, prioritisation and so on. This shouldn't take that much. My personal rule is that if it's going to take more than 2-3 h then I'm creating a task. This looks like a 15 mins one.

My recent experience is that it's not that easy. Sometimes we have to have additional entries to make it work (indirect dependencies). I'm happy to be wrong - that's just my observation from few months ago.

Re extra question. Unfortunately this is not that easy because of GHA and docker image used. If you convert insightsengineering/teal.* to teal.* then pre-commit will try to build docs using released version of packages. Then GHA will use devel version and auto-commit changes essentially overwritting what pre-commit did. These two has to be in sync. In my opinion we should switch to use released at some point. It's a much bigger problem that probably requires multiple changes here and there. I would refer you to this: https://github.com/insightsengineering/idr-tasks/issues/732

All in all - my ask is to just do this :) Let me close it