Skip to content
This repository has been archived by the owner on Dec 14, 2023. It is now read-only.

Bugfix missing sample files #75

Merged
merged 4 commits into from Sep 7, 2020

Conversation

jacobvjk
Copy link
Member

@jacobvjk jacobvjk commented Sep 1, 2020

After erroneously deleting the sample_files directory in a past clean up, this PR adds it again.
This folder contains the files required to set up a new PACTA_analysis project.

What happens when a new project is initialised via the project_settings.yml, is that the copy_files() function https://github.com/2DegreesInvesting/PACTA_analysis/blob/fd928ba2e520d91806ba93004203070ed004d2f1/0_global_functions.R#L184
will add some basic folder structure and parameter files in the target directory. These files are taken from the sample_files folder, that I removed by accident.

It's easy to validate by setting the project name in the project_settings.yml to something random such as test_12345 and twodii_internal to TRUE and running the first script. This creates the corresponding directory on the 2dii dropbox.

@jdhoffa @maurolepore

Copy link
Contributor

@maurolepore maurolepore left a comment

Choose a reason for hiding this comment

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

After erroneously deleting the sample_files directory in a past clean up, this PR adds it again.

Given this fixes an accident, it seems high priority and it will clearly improve the health of the system (which would otherwise be broken). Thus I'm quick to say LGTM (looks good to me) -- provided you confirmed this does -- as I suspect -- this breaks nothing unespected. 🚀

It's easy to validate ...

I didn't validate -- I have no Dropbox desktop on my Linux system. But please do validate yourself.

--

Nit: I see multiple dummy.txt files. I wonder if there is a more elegant way to solve this. Maybe some function checks if these folders exist; and if they don't, it creates them on the fly, right before the directory is actually needed.

@maurolepore
Copy link
Contributor

maurolepore commented Sep 1, 2020

@jacobvjk,
I prefer to not merge this myself because I lack context or tests. Unfortunately, you seem to lack admin privileges to merge your own PR. So let me know if this is an emergency, and we can have someone to approve me giving you the required permission.

@jacobvjk
Copy link
Member Author

jacobvjk commented Sep 1, 2020

@maurolepore I of course validated with the example I described above. But it's fine not to merge today. I will try to get @jdhoffa to do it tomorrow. It's urgent enough not to wait until Clare is back (in two weeks), but not urgent enough to warrant escalating it to emergency level today :)

Copy link
Member

@jdhoffa jdhoffa left a comment

Choose a reason for hiding this comment

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

Lgtm

@jacobvjk
Copy link
Member Author

jacobvjk commented Sep 7, 2020

@jdhoffa if you find time ti merge this today, that would be much appreciated. I don't have write access.

@jdhoffa jdhoffa merged commit b4b4bb2 into RMI-PACTA:master Sep 7, 2020
@maurolepore
Copy link
Contributor

Do we need to give @jacobvjk admin permission?
It seems useful and safe -- given he understand git well.

cc' @jdhoffa, @2diiKlaus, @Clare2D

@2diiKlaus
Copy link

Sounds reasonable to me. Especially in times when @Clare2D is on vacation.

@jacobvjk jacobvjk deleted the bugfix_missing_sample_files branch September 8, 2020 08:29
@jdhoffa
Copy link
Member

jdhoffa commented Sep 8, 2020

agreed, I think @jacobvjk should have admin permissions

@maurolepore
Copy link
Contributor

Thanks!

@jacobvjk, I increased your access permission. You now have rights to "write" to this repo. This is the minimum level I believe would give you the power you need. This can be changed at https://github.com/2DegreesInvesting/PACTA_analysis/settings/access.

image

pytonzlangen added a commit to pytonzlangen/PACTA_analysis that referenced this pull request Sep 10, 2020
@Clare2D
Copy link
Contributor

Clare2D commented Sep 14, 2020 via email

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants