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

49 update region isos #50

Merged
merged 22 commits into from May 4, 2020
Merged

49 update region isos #50

merged 22 commits into from May 4, 2020

Conversation

jdhoffa
Copy link
Member

@jdhoffa jdhoffa commented Apr 30, 2020

This is a draft since we will need to fix merge conflicts later when #48 is merged.
Closes #49

@jdhoffa jdhoffa marked this pull request as ready for review April 30, 2020 13:20
data-raw/region_isos.R Outdated Show resolved Hide resolved
data-raw/region_isos.R Outdated Show resolved Hide resolved
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.

I reviewed on GitHub's UI. Will use my own comments to explore deeper locally.

Also:

  • Add entry to NEWS.md

I'll hit "Request changes" but I might do them myself as I explore them locally.

@maurolepore
Copy link
Contributor

@jdhoffa, I see many lines overflow 80 characters -- which is a sane convention. Do you use a 80 character marker as a guide?

image

image

This seems like a minor issue but adds up to considerable effort. Many people like to keep their hands on the keyboard.

@maurolepore
Copy link
Contributor

maurolepore commented May 1, 2020

Thanks @jdhoffa! It's looking good. I have a few more questions / comments:

Are we okay to use data coming from a private source?

data-raw/region_isos.R Outdated Show resolved Hide resolved
@jdhoffa
Copy link
Member Author

jdhoffa commented May 4, 2020

btw we are ok to use this data publicly, yes!

This commit simplifies the file structure and code to read a
single raw region_isos dataset. This does the job for now so I
prefer to keep it simple.

But later we should need more complex code to store and read
multiple raw datasets (I think one each year). When that happens
we can revert this commit.

Also the new name of the raw data to more closely follows the
naming style of other datasets. The consistent prefix makes it
easier to see that region_isos.R and region_isos_* are related.
@maurolepore
Copy link
Contributor

maurolepore commented May 4, 2020

Thanks @jdhoffa!
I'll squash-merge this then imediately merge a patch to add 40b0333 to the trunk. That way that commit is independent and we can revert it on upstream whenever we need to store and read more than a single raw dataset region_isos.

(Just to clarify, what you see above is silly, I pushed the 40b0333 then reverted it with 9642a28. The effect is obviously the same as to never push 40b0333 -- but I thought of this "patch" design after I pushed. Sorry for the confusion.)

@maurolepore maurolepore merged commit f30a1fc into RMI-PACTA:master May 4, 2020
@jdhoffa jdhoffa deleted the 49-update-region_isos branch May 18, 2020 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Align region_isos to the definitions in the WEO2019
2 participants