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

Derive a replacement variable #162

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

nealrichardson
Copy link
Contributor

Sometimes when you combine categories or do some other derivation, you don't want to see the input variable anymore. But it's useful to keep it around so that if more data is appended, the derived variable updates.

This PR creates a means of attaching metadata to VariableDefinitions when creating them that indicates that, when they are added to the dataset, that some other variable(s) should be hidden and this new variable should appear in its place in the order.

This should also resolve/standardize some of the awkwardness in #154

@codecov
Copy link

codecov bot commented Nov 15, 2017

Codecov Report

Merging #162 into master will decrease coverage by 0.25%.
The diff coverage is 15.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #162      +/-   ##
==========================================
- Coverage   89.21%   88.95%   -0.26%     
==========================================
  Files          92       92              
  Lines        5479     5497      +18     
==========================================
+ Hits         4888     4890       +2     
- Misses        591      607      +16
Impacted Files Coverage Δ
R/add-variable.R 75.28% <6.25%> (-15.26%) ⬇️
R/combine-categories.R 97.66% <66.66%> (-0.56%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54fcd7c...0c2a098. Read the comment docs.

@nealrichardson nealrichardson mentioned this pull request Nov 16, 2017
Copy link
Contributor

@jonkeane jonkeane left a comment

Choose a reason for hiding this comment

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

The code looks good to me. I'm a little bit hesitant that replace might sound misleading to the user as being a literal replace the variable (especially with respect to the alias). I can see that making the replacement variable have the same alias as the original could break things like streaming and appends. So this is probably as good as we can get, and really as good as people need. But maybe a note saying that replace=TRUE will and must have a new alias that is unique.

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.

2 participants