Skip to content

Remove R development dependencies #4306

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.

Sign up for GitHub

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

Merged
bouweandela merged 18 commits into from
Jan 22, 2026
Merged

Remove R development dependencies #4306

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
6061efc
free two r deps
valeriupredoi Jan 8, 2026
16e8c6b
run GHA
valeriupredoi Jan 8, 2026
fb39f5f
pin r-base
valeriupredoi Jan 9, 2026
e4d2f83
Merge branch 'main' into free_some_deps
valeriupredoi Jan 19, 2026
7df1669
remove lintr and styler and dep for R env
valeriupredoi Jan 19, 2026
7de30de
re-turn on R linting via pre-commit
valeriupredoi Jan 19, 2026
d26a66f
skip precommit run in OSX
valeriupredoi Jan 19, 2026
70430d4
try 0.3.2
valeriupredoi Jan 19, 2026
4cdb832
0.3.2 is too old, trying 0.4.0
valeriupredoi Jan 19, 2026
a08ca4f
keep 0.4.0 but turn off lintr
valeriupredoi Jan 19, 2026
a2c5d63
bring back pre-commit config file to original state
valeriupredoi Jan 21, 2026
88f3e41
add to docs
valeriupredoi Jan 21, 2026
605fe08
turn off lintr test in linux too
valeriupredoi Jan 21, 2026
fdddae6
Remove outdated test
bouweandela Jan 22, 2026
ffa8401
Move pre-commit run to the right place
bouweandela Jan 22, 2026
a2d01d4
Merge branch 'main' into free_some_deps
valeriupredoi Jan 22, 2026
6952ee3
unrun GHA tests
valeriupredoi Jan 22, 2026
2f0d750
Add caption
bouweandela Jan 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ on:
push:
branches:
- main
- free_some_deps
schedule:
- cron: "0 0 * * *"

Expand Down
6 changes: 3 additions & 3 deletions environment.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ dependencies:

# R and dependencies
- cdo
- r-base >=3.5
- r-base >=4.4
- r-abind
- r-akima
- r-climdex.pcic
Expand All @@ -123,5 +123,5 @@ dependencies:
- r-yaml
# R packages needed for development
- r-git2r # dependency of lintr
- r-lintr ==3.1.2
- r-styler ==1.10.3
Comment thread
valeriupredoi marked this conversation as resolved.
- r-lintr
- r-styler

@bouweandela bouweandela Jan 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should be able to get rid of those if we re-enable the pre-commit hook at

ESMValTool/.pre-commit-config.yaml

Lines 45 to 53 in 9bde7d3

# - repo: https://github.com/lorenzwalthert/precommit/ # Checks for R
# rev: 'v0.4.2'
# hooks:
# - id: style-files # styler
# - id: lintr
# - repo: https://github.com/codespell-project/codespell
# rev: 'v2.3.0'
# hooks:
# - id: codespell

and perform the currently failing test using pre-commit instead.

We would just need to pin that pre-commit hook to the version where the checks passed (i.e. the version of the pre-commit hook that uses the versions of lintr and styler listed here).

@valeriupredoi valeriupredoi Jan 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

all fun and games until I realized I lost you at pinning the pre-commit hook - how the heck would we know the version when that hook last passed the test? Unless you want me to write a Monte Carlo routine that loops over lintr hook versions 🤣

@valeriupredoi valeriupredoi Jan 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

can we not just skip the R-linting test in tests/unit/test_lint.py? I've not seen any of the following in the past 5 years:

  • anyone writing a recipe in R
  • anyone complaining that our R recipes don't follow R linting standards

@valeriupredoi valeriupredoi Jan 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

actually, scratch that - it's prob a good idea to have an r-lintr test, let me see what I can do with that pre-commit version, I'll toss some coins 🪙

@valeriupredoi valeriupredoi Jan 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've not forgot about this, am gonna try have a look at it tomorrow - sorry, I've been under a lot of snow elsewhere, bud

@valeriupredoi valeriupredoi Jan 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

pre-commit config file brought back to original state in a2c5d63 - off to the docs to add the mention about style etc

@valeriupredoi valeriupredoi Jan 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

docs modded in 88f3e41 - do we want to tell R folks to use the pre-commit way or r-linrs/styler way?

@bouweandela bouweandela Jan 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the changes V! I pushed a few additions to the docs in fdddae6 and removed the unit test that checks compliance with lintr as we want this done with pre-commit instead.

@valeriupredoi valeriupredoi Jan 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Looks great! I had doubts about keeping that fully skipped test, but I was afraid we'd have to use it again in the future should the R hook be left undeveloped (it diesn't look too actively maintained). Shall we merge then? 🍺

@bouweandela bouweandela Jan 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, but let's first fix the docs build

Loading