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

internal: replace magrittr pipe with base pipe #189

Open
francisbarton opened this issue Sep 11, 2023 · 6 comments
Open

internal: replace magrittr pipe with base pipe #189

francisbarton opened this issue Sep 11, 2023 · 6 comments
Labels
documentation Improvements or additions to documentation enhancement New feature or request

Comments

@francisbarton
Copy link
Contributor

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
Copy link
Collaborator

ThomUK commented Nov 21, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

francisbarton commented Aug 5, 2024

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
Copy link
Member

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.

@Lextuga007 Lextuga007 added documentation Improvements or additions to documentation enhancement New feature or request labels Oct 7, 2024
@tomjemmett
Copy link
Member

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants