search

nhs-r-community / NHSRplotthedots

An SPC package to support NHSE/I 'Making Data Count' programme
https://nhs-r-community.github.io/NHSRplotthedots/
Other
48 stars 23 forks source link

internal: replace magrittr pipe with base pipe #189

Open francisbarton opened 1 year ago

francisbarton commented 1 year ago

If the dev team agrees with this then I can do the editing work to replace the pipes. It will require some changes to the way the mockery tests are written as well as any formula type bits of code with ~ . I am posting this as an issue as a first step towards creating a PR - but if the general step is not approved then there is no point making the changes.

ThomUK commented 1 year ago

I for one would support this change in principle, as it helps ensures users are moving to recent versions of R, which is a good thing. However I'm unsure of priority vs other issues, as it adds no end-user benefit. If an enabling change for other features this would work in this issue's favour. Interested in other views.

francisbarton commented 1 year ago

The base pipe is already in the package in some places, after recent commits, and so the min R version is in the DESCRIPTION too.

Take your point about cost:benefit - there's a risk that doing this work breaks something and generates more work. But I think it should in most cases be fairly straightforward swapping from magrittr to base.

francisbarton commented 4 months ago

I'm still in favour of doing this and happy to action it if the dev team are in agreement. Will do this once currently open PRs are dealt with, otherwise things could get very messy!

Lextuga007 commented 1 month ago

I'm in favour of this as it reduces a dependency. It'll be a good practice for refactoring and using/updating tests. Also pinging @tomjemmett for views on this.

tomjemmett commented 1 month ago

This would mean breaking support for R < 4.1? Whenever it was the base pipe was introduced. Not sure if that's a big enough issue or not!

Lextuga007 commented 1 month ago

Given the vulnerability mentioned for versions up to and including 4.4.0 we should update the R version. Although it's officially awaiting analysis https://nvd.nist.gov/vuln/detail/CVE-2024-27322 it is an important reason to encourage organisations and IT teams to allow for R programs to be updated.