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

lfe-tidier: add argument quick to tidy #509

Merged
merged 5 commits into from Oct 22, 2018
Merged

lfe-tidier: add argument quick to tidy #509

merged 5 commits into from Oct 22, 2018

Conversation

MatthieuStigler
Copy link
Contributor

Hi

tidy.felm did not have the quick option, which was very useful in my case. I made an implementation of it, trying to keep code as in other functions. I did local test, but no global ones (difficult to run R CMD check as so many packages required)... I hope it's fine!?

Thanks!

@alexpghayes
Copy link
Collaborator

This looks great! Can you add some tests for tibble output when quick = TRUE? Or just construct a tibble instead of a data.frame?

@MatthieuStigler
Copy link
Contributor Author

ok, I did following changes:

  • use directly data_frame
  • added tests with quick=TRUE in test-lfe.R

I also corrected my own change, where now quick takes precedence over conf.int=TRUE (i.e. if (conf.int) becomes if (!quick & conf.int)). This seemed to be the policy in broom, am I correct?

Thanks

@alexpghayes
Copy link
Collaborator

Looks good, thanks! Will you update NEWS and add yourself as a contributor in DESCRIPTION?

@MatthieuStigler
Copy link
Contributor Author

ok, I did! It seems now it is not passing the test, but seems for a totally different reason?

@github-actions
Copy link

This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants