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

Pre-release activities - Potential removal of dependencies #643

Closed
8 tasks done
averissimo opened this issue Feb 19, 2024 · 7 comments · Fixed by #663
Closed
8 tasks done

Pre-release activities - Potential removal of dependencies #643

averissimo opened this issue Feb 19, 2024 · 7 comments · Fixed by #663
Assignees
Labels
core enhancement New feature or request

Comments

@averissimo
Copy link
Contributor

averissimo commented Feb 19, 2024

Part of #624 related to vignettes. Please make a PR to pre-release@main

Consider:

  • Removal of {magrittr} by importing pipe operator from {dplyr} ✔️ done
  • Other packages that can be easily removed ✔️ removed tidyselect as its functions are reexported from dplyr
  • Move any of the packages from Depends -> Imports
    • {ggmosaic}impossible (see below)
    • {ggplot2}impossible (see below)
    • {shinyTree} ✔️ done
    • {teal.transform}valid: teal.transform functions should be available to tmg users
    • {teal}valid: teal dependency is standard in all module packages
    • {shiny}valid: like teal, module packages use shiny extensively
@averissimo averissimo added enhancement New feature or request core labels Feb 19, 2024
@averissimo averissimo mentioned this issue Feb 19, 2024
26 tasks
@chlebowa chlebowa self-assigned this Feb 21, 2024
@chlebowa
Copy link
Contributor

ggmosaic must stay in Depends as per this note in teal.modules.general.R:

# nolint start
# Note ggmosaic (version <= 0.3.3) needs to be in DEPENDS as the following does not work if it is imported
# df <- data.frame(x = c("A", "B", "C", "A"), y = c("Z", "Z", "W", "W"))
# ggplot(df) + ggmosaic::geom_mosaic(aes(x = ggmosaic::product(x), fill = y))
# nolint end

I experimented with moving it to Imports and it seems to be valid.

ggplot2 will remain in Depends as well as long as ggmosaic has to. It makes no sense to me to have a basic funcitonality in Imports and an enhancement in Depends.

@chlebowa
Copy link
Contributor

I would actually consider adding @import stats as there are numerous uses of multiple functions. Not necessarily, though, prefixes do add clarity.

The same goes for teal.widgets.

@chlebowa
Copy link
Contributor

nestcolor is used in most (if not all) examples but nowhere else. I believe it should be upgraded from Suggests to Imports because we don't want those examples to be made conditional.
Alternatively, we can use require(nestcolors) rather than library(nestcolors) in examples.

Thoughts?

@chlebowa
Copy link
Contributor

teal.data should be upgraded from Suggests to Imports as datanames and join_keys is used in many modules, aside from examples.

@chlebowa
Copy link
Contributor

done

@pawelru
Copy link
Contributor

pawelru commented Feb 22, 2024

Can you please check if this solves #625. If yes - please close

@chlebowa
Copy link
Contributor

Yes, I got no notes on my last pass.
image

averissimo added a commit that referenced this issue Feb 26, 2024
# Pull Request

Part of #624

Follow-up to:

- #643

### Changes description

- Removes `magrittr` from `Config/Needs/verdepcheck`
- Re-order `verdpecheck` with same order on `Depends`, `Imports`,
`Suggests`
- Re-order `pre-commit` config with same order on `Depends`, `Imports`,
`Suggests`
- Add remote repo to `htmlwidgets/sparkline` (despite not being updated
in years) in `verdepcheck`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants