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

bugfixes for Rcpp::exposeClass #886

Merged
merged 5 commits into from Jul 25, 2018

Conversation

Projects
None yet
4 participants
@mlysy
Contributor

mlysy commented Jul 24, 2018

Attempts to fix issue #879 and a couple of other minor issues with Rcpp::exposeClass, namely:

  • loadRcppClass (called by exposeClass) handles the Class and CppClass arguments correctly now, such that the R and C++ class names for a given module needn't be the same.
  • The rename argument to exposeClass now functions properly.
  • When exposeClass is called in the root directory of a package, the generated wrapper code is put in src and R subdirectories. This behavior is now ignored if file or Rfile contain absolute paths, in which case the files are written there.

A basic unit test for these changes (inst/unitTests/runit.exposeClass.R) is provided.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jul 24, 2018

Member

Wow. Thanks! Will study. Could you possibly rebase with master? Your ChangeLog shows you branched after July 12 so you are missing a little bit. It's not critical -- I could always squash-merge as well. (And will hand-adjust ChangeLog.)

Member

eddelbuettel commented Jul 24, 2018

Wow. Thanks! Will study. Could you possibly rebase with master? Your ChangeLog shows you branched after July 12 so you are missing a little bit. It's not critical -- I could always squash-merge as well. (And will hand-adjust ChangeLog.)

@mlysy

This comment has been minimized.

Show comment
Hide comment
@mlysy

mlysy Jul 24, 2018

Contributor

Sure! (I just distractedly updated only the ChangeLog, here comes the rest...)

Contributor

mlysy commented Jul 24, 2018

Sure! (I just distractedly updated only the ChangeLog, here comes the rest...)

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jul 24, 2018

Member

Interesting changes, eg the Class vs CppClass. Were we operating on the wrong variable? Entirely possible, but also possible this may have side effects :) I trust you tested this?

Member

eddelbuettel commented Jul 24, 2018

Interesting changes, eg the Class vs CppClass. Were we operating on the wrong variable? Entirely possible, but also possible this may have side effects :) I trust you tested this?

@mlysy

This comment has been minimized.

Show comment
Hide comment
@mlysy

mlysy Jul 24, 2018

Contributor

I reran all unitTests and added my own. (There were some errors but not due to my additions; I didn't fix them to focus on the issue the PR is attempting to solve...)

Contributor

mlysy commented Jul 24, 2018

I reran all unitTests and added my own. (There were some errors but not due to my additions; I didn't fix them to focus on the issue the PR is attempting to solve...)

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jul 24, 2018

Codecov Report

Merging #886 into master will increase coverage by 0.09%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #886      +/-   ##
==========================================
+ Coverage   90.09%   90.18%   +0.09%     
==========================================
  Files          71       71              
  Lines        3271     3271              
==========================================
+ Hits         2947     2950       +3     
+ Misses        324      321       -3
Impacted Files Coverage Δ
R/RcppClass.R 73.75% <50%> (+3.75%) ⬆️

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 12f3c03...586d4db. Read the comment docs.

codecov-io commented Jul 24, 2018

Codecov Report

Merging #886 into master will increase coverage by 0.09%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #886      +/-   ##
==========================================
+ Coverage   90.09%   90.18%   +0.09%     
==========================================
  Files          71       71              
  Lines        3271     3271              
==========================================
+ Hits         2947     2950       +3     
+ Misses        324      321       -3
Impacted Files Coverage Δ
R/RcppClass.R 73.75% <50%> (+3.75%) ⬆️

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 12f3c03...586d4db. Read the comment docs.

@mlysy

This comment has been minimized.

Show comment
Hide comment
@mlysy

mlysy Jul 24, 2018

Contributor

Hopefully this the correct rebase, modulo ChangeLog.

Contributor

mlysy commented Jul 24, 2018

Hopefully this the correct rebase, modulo ChangeLog.

@kevinushey

This comment has been minimized.

Show comment
Hide comment
@kevinushey

kevinushey Jul 24, 2018

Contributor

Awesome! Thanks for the PR.

While I'm not that familiar with the modules codebase, all of the changes here look sensible and correct, so a thumbs up from me.

Contributor

kevinushey commented Jul 24, 2018

Awesome! Thanks for the PR.

While I'm not that familiar with the modules codebase, all of the changes here look sensible and correct, so a thumbs up from me.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jul 24, 2018

Member

Same here. Will run a rev.dep check just in case.

Member

eddelbuettel commented Jul 24, 2018

Same here. Will run a rev.dep check just in case.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jul 25, 2018

Member

Sorry, got distracted for a bit. Reverse dependency check threw up nothing new, so merging this now.

Member

eddelbuettel commented Jul 25, 2018

Sorry, got distracted for a bit. Reverse dependency check threw up nothing new, so merging this now.

@eddelbuettel eddelbuettel merged commit e150eba into RcppCore:master Jul 25, 2018

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