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

mapply was simplifying list of single row data tables to a matrix #263

Merged
merged 4 commits into from Jan 14, 2020

Conversation

@TylerGrantSmith
Copy link
Contributor

@TylerGrantSmith TylerGrantSmith commented Jan 4, 2020

Addresses issue #260

@codecov-io
Copy link

@codecov-io codecov-io commented Jan 4, 2020

Codecov Report

Merging #263 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #263      +/-   ##
==========================================
+ Coverage   92.44%   92.46%   +0.01%     
==========================================
  Files          12       12              
  Lines         927      929       +2     
==========================================
+ Hits          857      859       +2     
  Misses         70       70
Impacted Files Coverage Δ
R/testing_utils.R 82.85% <100%> (+0.5%) ⬆️
R/FunctionReporter.R 93.25% <100%> (+0.02%) ⬆️

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 2f43f73...ef9361e. Read the comment docs.

@jameslamb jameslamb requested review from jayqi, bburns632 and jameslamb Jan 4, 2020
@jameslamb
Copy link
Member

@jameslamb jameslamb commented Jan 4, 2020

@TylerGrantSmith thanks! Could you elaborate on what the issue is? Did that simplification result in an error message or inaccurate result?

Also, do you have a reproducible example that we could turn into a unit test, to prevent us from reintroducing the issue in the future?

@TylerGrantSmith TylerGrantSmith force-pushed the TylerGrantSmith:bug/issue-260 branch from 2031538 to 5cc4827 Jan 5, 2020
Copy link
Collaborator

@jayqi jayqi left a comment

Thanks so much for this contribution, @TylerGrantSmith! I especially like the new silverstein test package. A few minor revisions requested.

tests/testthat/test-FunctionReporter-class.R Outdated Show resolved Hide resolved
tests/testthat/test-FunctionReporter-class.R Outdated Show resolved Hide resolved
inst/silverstein/R/Carrots.R Outdated Show resolved Hide resolved
inst/silverstein/DESCRIPTION Show resolved Hide resolved
R/FunctionReporter.R Show resolved Hide resolved
@jayqi jayqi self-requested a review Jan 14, 2020
@jayqi
jayqi approved these changes Jan 14, 2020
Copy link
Collaborator

@jayqi jayqi left a comment

LGTM. Thanks @TylerGrantSmith!

@jayqi jayqi merged commit f3ec7e4 into uptake:master Jan 14, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.