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

Pipeline crosstab #34

Merged
merged 8 commits into from Jul 8, 2016
Merged

Pipeline crosstab #34

merged 8 commits into from Jul 8, 2016

Conversation

@chrishaid
Copy link
Collaborator

@chrishaid chrishaid commented Jul 7, 2016

This creates an S3 generic for crosstab as well as default and data.table methods. The crosstab is now pipeable. One side effect of this is I had to allow list objects as acceptable objects for vec1 and vec2 parameters in the crosstab.default. (Essentially as a way to deal with NSE in both the crosstab.data.frame and crosstab.default methods). I've updated the tests and the package is currently passing R CMD CHECK on my MBP.

@codecov-io
Copy link

@codecov-io codecov-io commented Jul 7, 2016

Current coverage is 98.82%

Merging #34 into master will decrease coverage by 1.17%

@@           master        #34   diff @@
========================================
  Files           9          9          
  Lines         151        170    +19   
  Methods         0          0          
  Messages        0          0          
  Branches        0          0          
========================================
+ Hits          151        168    +17   
- Misses          0          2     +2   
  Partials        0          0          

Powered by Codecov. Last updated by 8490213...3844188

@sfirke
Copy link
Owner

@sfirke sfirke commented Jul 8, 2016

Wow! Wizardry. I don't entirely follow and will need to do some reading to 100% understand, but it works great.

The one question I have now is, I see when I start typing "crosstab" in RStudio I get "crosstab.data.frame" and "crosstab.default" suggested. Is it standard to export those? Is it possible some users will want to call them directly? I don't immediately see a purpose to exporting them.

@sfirke sfirke merged commit fe67ea8 into sfirke:master Jul 8, 2016
1 of 3 checks passed
1 of 3 checks passed
codecov/patch 93.93% of diff hit (target 100%)
Details
codecov/project 98.82% (-1.18%) compared to 8490213
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@chrishaid
Copy link
Collaborator Author

@chrishaid chrishaid commented Jul 8, 2016

Your probably right regarding exporting them. I'll double check tomorrow and make changes as necessary.

@almartin82
Copy link
Collaborator

@almartin82 almartin82 commented Jul 9, 2016

I don't think you generally export the functions that only do method dispatch. I think those stay internal

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.