Update How to Install and Use Geospatial R Packages.md

Public-Health-Scotland / technical-docs

Technical documentation, including guidance and best practice for Public Health Scotland (PHS)

https://public-health-scotland.github.io/knowledge-base/docs

5 stars 2 forks source link

Update How to Install and Use Geospatial R Packages.md #112

Closed Moohan closed 2 months ago

Moohan commented 4 months ago

I added some text / code on how to update your/a project's .Rprofile

The guide includes a few bits of code that need to be run in every session to make geospatial packages work. It does at one point suggest adding code to a porject's .Rprofile but it doesn't specify how to do this. The other issue is that the code that should be included in an .Rprofile is split accross the guide with the env variables at the top and the DLL loads at the bottom, so it would be easy for someone to miss this and only add part of the required code to an .Rprofile.

I also included some code to 'automatically' add the text to a (personal) .Rprofile, although I'm in two minds as to whether this lowers the bar 'too much'.

Moohan commented 3 months ago

Am I ok to merge this?

I checked the contribution guidelines and there's not really a recommended flow for PRs. I would suggest:

Require a 2 approved reviews (add branch protection)
The second approver should merge (or squash and merge?) the PR, or should tag a third reviewer in a comment (if required), the third reviewer should merge on approval or ...
Some people (DS team?) should be able to override these requirements to be able to push through very simple PRs (typo fixing etc.) - Also done with branch protection.

terrymclaughlin commented 3 months ago

@Moohan

There's branch protection enabled on the main branch, requiring a pull requested and 1 approval before merging. I have increased this to 2 approvals.

Moohan commented 3 months ago

@terrymclaughlin I just tested both of these cases (I switched to 3.6.1 where I have no packages installed which I assume is sufficient?), and it works fine. The only lines I was worried about were the dyn.load(...) but it seems as with the other bits that code can all just run but is only relevant when you try to load / install a geospatial package.