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

Rework of summary_factorlist() to reduce code duplication #14

Closed
wants to merge 1 commit into from

Conversation

@onesandzeroes
Copy link

@onesandzeroes onesandzeroes commented Feb 16, 2019

I was looking at the source for summary_factorlist this week to see how easy it would be to add a feature to it, and noticed there was a bit of code duplication with summary_factorlist1, summary_factorlist2, etc. This PR is an attempt to rewrite some of that implementation with code that is (hopefully) cleaner and more maintainable. Obviously it uses dplyr/tidyr heavily but they were already imports for this package so hopefully that's OK.

The PR may not be ready to merge just yet, I just wanted to see if you would be open to this kind of change. Happy to address any issues you may have.

I've checked to make sure all tests and CRAN checks pass with the reworked code, and have done some additional testing to make sure the output is the same before and after.

Copy link
Owner

@ewenharrison ewenharrison left a comment

Great, many thanks for this. This was on the list to do! I will have a look.
I had also intended to change plyr::ldply to purrr::mapto remove the plyr dependency.

@ewenharrison
Copy link
Owner

@ewenharrison ewenharrison commented Feb 18, 2019

Hi Marius,
Thanks again for this. A bit of work is needed make sure there are not issues. Can you do a pull request to the branch summary_factorlist_rework , I'll merge to that and we can work from there?

ewenharrison added a commit that referenced this issue Feb 21, 2019
ewenharrison added a commit that referenced this issue Feb 21, 2019
ewenharrison added a commit that referenced this issue Feb 22, 2019
@ewenharrison
Copy link
Owner

@ewenharrison ewenharrison commented Feb 22, 2019

This is done from #15

ewenharrison added a commit that referenced this issue Feb 22, 2019
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

2 participants