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

augment method for chi-squared test #138

Merged
merged 3 commits into from Jun 7, 2018
Merged

Conversation

larmarange
Copy link
Contributor

@benmarwick
Copy link

benmarwick commented Aug 10, 2016

Thanks for proposing this, it would be an excellent addition to the package. It would be a huge improvement over wrangling the output from chisq.test or descr::CrossTable. I hope this method can be added soon.

@alexpghayes
Copy link
Collaborator

If you're up for it amidst all the other broom work you're currently doing, I'd love to get this merged too!

@larmarange
Copy link
Contributor Author

larmarange commented Jun 7, 2018

Dear @alexpghayes ,

I have rebased the pull request and I have tried to follow your vignette https://github.com/tidyverse/broom/blob/master/vignettes/adding-tidiers.Rmd

Some comments:

  • I have applied styler only to the R files I have changed. But it seems that some other R files in broom do not respect styler. Maybe it could be relevant to clean the code and apply once styler to the overall package. By the way, it could be relevant to add a mention to styler package in your vignette.
  • I have also run goodpractice:gp() as requested but I have fixed only the comments relative to the R files I have amended. There is a lot of remarks regarding broom (mainly lines > 80 characters, the use of sapply, and the form 1:nrow(x)). Maybe it would be relevant to clean a little the current code if you want to ask external contributors to use systematically gp().
  • It is not mentioned in your vignette if NEWS.md should be updated or not. I have updated it but let me know if it should not be part of the PR.
  • When applying pkgdown::build_site() I encountered some errors and I have changed _pkgdown.yml to solve that problem (cf. 6a764df) but let me know if it should be removed from the PR.
  • In the vignette, it could be relevant to specify that for non US or UK developpers, it is necessary to execute Sys.setlocale("LC_ALL", "English") before pkgdown::build_site() (otherwise, messages in the vignettes will be in another language than English).
  • I have enclosed all changes due to pkgdown::build_site() in a separate commit although it was not specified in your vignette if you preferred a unique commit.
  • You will see that the execution of pkgdown::build_site() is generating file conflicts. This is due to the fact that this command is updating all the website and therefore all pages (just because date as changed for example). I do not know if it is a good idea to include pkgdown::build_site() in all PR as some conflicts could appear only because the website was updated since the PR was prepared. Maybe devtools::document() could be enough for a PR (as it will be enough for checking the package)?

Anyway, please let me known what are the changes you expect before an eventual integration of the PR. I prefer to finalise that one and to be sure to have understood all the elements you expect before preparing the other PR we discussed.

Best regards

@larmarange
Copy link
Contributor Author

OK.

I have rebased once again the PR from upstream/master and re-run pkgdown::build_site()

Let me know if any additional changes are required in the PR.

Regards

@alexpghayes
Copy link
Collaborator

Wow, this is fantastic feedback! I'll update the vignette on adding new tidiers later today -- thank you so much!

Everything you've done is totally reasonable. I'll try to get styler and gp cleaned up for the whole package, but it make take a little while.

@larmarange
Copy link
Contributor Author

larmarange commented Jun 7, 2018

I completely understand.

Regarding the vignette (as I'm sure it will be a question for several occasional developers), it would be great if it could specified:

  • if you want all the commits of the PR squashed into one or not;
  • if you prefer build_site to be in a separate commit.

I do not know if it is important to explain all that process with the corresponding git commands. A link to https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request could be useful or a dedicated paragraph in the vignette.

Regards

@alexpghayes alexpghayes merged commit df94772 into tidymodels:master Jun 7, 2018
@larmarange larmarange deleted the chisq branch June 7, 2018 18:33
@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 13, 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

3 participants