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

Add new step_indicate_na() #623

Merged
merged 5 commits into from
Feb 3, 2021

Conversation

konradsemsch
Copy link

That's a clear start from: #617

@konradsemsch konradsemsch mentioned this pull request Dec 18, 2020
@juliasilge juliasilge changed the title added files Add new step_indicate_na() Dec 18, 2020
Copy link
Member

@topepo topepo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

R/naindicate.R Outdated Show resolved Hide resolved
@topepo
Copy link
Member

topepo commented Dec 18, 2020

One more thing that I forgot... please add a note to the NEWS file.

@konradsemsch
Copy link
Author

Done, should be good to go!

@juliasilge
Copy link
Member

@konradsemsch It seems that in the last commit f9cd575 a whole lot of unrelated files got checked in. We only want the files related to this PR checked in, not the .Rd files for other steps, and for example no step_shadow_missing methods in the NAMESPACE. Do you think you can give a go at fixing up this PR?

@konradsemsch
Copy link
Author

I just spotted that in the R file:

#' @export
tidy.step_shadow_missing <- function(x, ...) {
  if (is_trained(x)) {

and let me remove it, before I make the change. I think I also committed everything after loading the package locally so let me revert that.

@konradsemsch
Copy link
Author

@juliasilge let me know if things are as you would expect and sorry about the previous push clutter 😉

@topepo
Copy link
Member

topepo commented Dec 29, 2020

Are all of the changes in for this PR?

@konradsemsch
Copy link
Author

All from my side :)

Copy link
Member

@topepo topepo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Sorry for the lag time. One minor change.

R/naindicate.R Outdated Show resolved Hide resolved
@topepo
Copy link
Member

topepo commented Feb 3, 2021

Thanks!

@topepo topepo merged commit 1e2f318 into tidymodels:master Feb 3, 2021
@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 reprex) and link to this issue. https://reprex.tidyverse.org

@github-actions github-actions bot locked and limited conversation to collaborators Feb 20, 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.

3 participants