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

New option allows reserved loanbook columns #328

Merged
merged 5 commits into from
Nov 30, 2020
Merged

New option allows reserved loanbook columns #328

merged 5 commits into from
Nov 30, 2020

Conversation

maurolepore
Copy link
Contributor

@maurolepore maurolepore commented Nov 27, 2020

Closes #326
Closes #327 -- this is a simpler/safer way to solve it.
Relates to:

  • 2DegreesInvesting/r2dii.analysis#227
  • 2DegreesInvesting/r2dii.analysis#230

This change is internal, and motivated by our need to break the
dependency between r2dii.data and its reverse dependencies. The
problem we are trying to solve is this:

r2dii.data provides data that is used in some functions of its
reverse dependencies -- like r2dii.match. Concretely, changes in
the sector classification dataset (in r2dii.data) make tests to
fail in reverse dependencies like r2dii.match that store a
snapshot of expected output (e.g. of match_name()).

This commit allows us to generate such snaphsots with a
"frozen loanbook" -- one that does not update with changes
to r2dii.data and instead has fixed columns sector and
borderline`. This makes r2dii packages less prone to
meaningless errors when CRAN checks for reverse dependencies.

Closes #326
Closes #327 -- this is a simpler/safer way to solve it.
Relates to:
* https://github.com/2DegreesInvesting/r2dii.analysis/pull/227
* https://github.com/2DegreesInvesting/r2dii.analysis/pull/230

This change is internal, and motivated by our need to break the
dependency between r2dii.data and its reverse dependencies. The
problem we are trying to solve is this:

r2dii.data provides data that is used in some functions if its
reverse dependencies -- like r2dii.match. Concretely, changes in
the sector classification dataset (in r2dii.data) make tests to
fail in reverse dependencies like r2dii.match that store a
snapshot of expected output (e.g. of match_name()).

This commit allows us to use generate such snaphsots with a
frozen loanbook, and thus make r2dii packages less prone to
meaningless errors when CRAN checks for reverse dependencies.
It's best practice to use on.exit() with add = TRUE and
after = FALSE; or instead to use withr::defer(). But add = TRUE
was added after 3.4, and I want to avoid a dependency on withr.
So I just go with on.exit() and add = TRUE, as this only really
matters when you call on.exit() more than once.
@maurolepore maurolepore marked this pull request as ready for review November 27, 2020 19:42
@maurolepore maurolepore requested a review from jdhoffa November 27, 2020 19:42
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

@jdhoffa jdhoffa merged commit 93d4dd4 into master Nov 30, 2020
@jdhoffa jdhoffa deleted the bypass branch November 30, 2020 09:48
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.

match_name should allow loanbook_demo to have reserved columns sector and borderline
2 participants